[Insight-developers] ImageIterators : Suggested Patch: Exception

Luis Ibanez luis.ibanez at kitware.com
Fri May 8 08:13:13 EDT 2009 

Gaetan,

That's great.

Thanks for looking at this problem.

We will be following on the other filters
(the ones not related to morphology)


        Luis


-----------------------------------------------
2009/5/8 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>
> Hi Luis,
>
> I'll try to fix that this weekend, and to fix the various problems found in
> those classes as well.
>
> Regards,
>
> Gaëtan
>
>
> Le 6 mai 09 à 13:15, Luis Ibanez a écrit :
>
>>
>> Hi Gert,
>>
>> I like the option of having a Macro that
>>
>> * in Debug mode will use assert
>> * in Release mode will throw and exception.
>>
>>
>> --------
>>
>>
>> Coming back to specific problem at hand:
>>
>>
>>    A Nightly build was run with this
>>    patch for Iterators and 26 tests failed:
>>
>> http://www.cdash.org/CDash/viewTest.php?onlyfailed&buildid=327351
>>
>> About 12 of them are related to Mathematical Morphology classes
>> in Code/Review.
>>
>> Others are in unexpected places, like:
>>
>>  * itkImageSeriesWriterTest
>>  * itkCommonPrintTest
>>
>>
>>
>> It seems that this is a fundamental flaw that we want to
>> fix before releasing ITK 3.14 by the end of this month.
>>
>>
>>
>>  Luis
>>
>>
>> -------------------
>> Gert Wollny wrote:
>>>
>>> On Tue, 2009-05-05 at 19:53 +0200, Gaëtan Lehmann wrote:
>>>>
>>>> Hi,
>>>>
>>>> It would be nice to have that check in the code.
>>>> I think it's better to not use assert() here because it can let the
>>>>  user to catch problematic mistakes in it's code and the check is not  so
>>>> computing intensive, so I doubt we can see any difference.
>>>
>>> Well, since the error is a programing error, the reason to use assert
>>> over throwing an exceptions is that with an assertion, you can open a
>>> debugger and end up exactly at the offending code line [1] - either by
>>> starting the debugger directly, like Visual Studio does, or by examining
>>> the core dump on a Unix-like system, and you can see directly, where the
>>> bad input came from. When an exception is thrown, the stack is unwind,
>>> and you have a hard
>>> time to find the source of the offending data. The only approach here is
>>> to explicitly open a debugging session, and then configure the debugger
>>> to catch exceptions when they are thrown.
>>> One idea to check also in the release builds would be that in debug
>>> builds the code uses assert, and in release builds it throws an
>>> exception, so that a developer can get easily to the offending
>>> code/data, and a user sees a reasonable error message without a core
>>> dump or general protection fault. The other option would be to define an
>>> assert that is also compiled in when NDEBUG is defined.
>>> [1] H. Sutter & A. Alexandrescu, "C++ Coding Standards"
>>> Regards, Gert
>>>>
>>>> Regards,
>>>>
>>>> Gaëtan
>>>>
>>>>
>>>>
>>>> Le 5 mai 09 à 18:14, Luis Ibanez a écrit :
>>>>
>>>>
>>>>> Hi Brad,
>>>>>
>>>>>
>>>>> Thanks for the feedback.
>>>>>
>>>>>
>>>>> We don't have guidelines for asserts nor preconditions in ITK.
>>>>>
>>>>>
>>>>> Although,
>>>>> unless I'm missing something,
>>>>> that will simply mean that instead of writing:
>>>>>
>>>>>
>>>>> if( ! bufferedRegion.IsInside( m_Region ) )
>>>>>   {
>>>>>   itkGenericExceptionMacro("..");
>>>>>   }
>>>>>
>>>>>
>>>>> we will write
>>>>>
>>>>>
>>>>> assert( bufferedRegion.IsInside( m_Region ) )
>>>>>
>>>>>
>>>>> and the implementation of assert witll throw the exception.
>>>>>
>>>>> Which is admitedly a more elegant way of writing the code.
>>>>>
>>>>>
>>>>> We could achieve a similar goal with itkTestingMacros such
>>>>> as the ones we have been using in
>>>>>
>>>>>
>>>>> http://svn.na-mic.org/NAMICSandBox/trunk/QuadEdgeMeshRigidRegistration/Testing/itkTestingMacros.h
>>>>>
>>>>>
>>>>> Would you recommend a better approach ?
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>   Luis
>>>>>
>>>>>
>>>>>
>>>>> ------------------------
>>>>> Bradley Lowekamp wrote:
>>>>>
>>>>>> I ran into this issue with trying to improve the ShrinkImageFilter.  I
>>>>>> think this is a good check to make. In my case we only had test  failing on
>>>>>> one or two systems, even through the buffer was being  accessed outside the
>>>>>> buffered region.
>>>>>> However, this seems like an assertion of preconditions. In this  case
>>>>>> the precondition is that the region parameter must be with in  the buffered
>>>>>> region. There are a variety of ways to verify  preconditions. One way is to
>>>>>> use an assert type macro that is only  implemented in Debug versions. Do we
>>>>>> have any guidelines for  asserts or verification of preconditions in ITK?
>>>>>> Brad
>>>>>> On May 5, 2009, at 11:07 AM, Luis Ibanez wrote:
>>>>>>
>>>>>>> With Michel Audette we have been tracking the issues related to
>>>>>>> aligning physical point coordinates with index coordinates.
>>>>>>>
>>>>>>> In the process we noticed that the ITK image iterators do not
>>>>>>> verify at construction time whether the region given as argument
>>>>>>> to the iterator constructor,  is inside of the image or not.
>>>>>>>
>>>>>>> The consequence is that a user can provide a region outside
>>>>>>> of the image, we compute the offset (that will turn out to be
>>>>>>> outside of the image buffer) and then we (incorrectly) provide
>>>>>>> access to the pixel location.
>>>>>>>
>>>>>>> This has now been reported as Bug #8960:
>>>>>>>
>>>>>>>  http://public.kitware.com/Bug/view.php?id=8960
>>>>>>>
>>>>>>>
>>>>>>> We are suggesting the following fix to this problem:
>>>>>>>
>>>>>>> Namely, to check the region argument of the iterator constructor
>>>>>>> against the BufferedRegion of the image, and throw an exception
>>>>>>> if the region is not fully inside of the image buffered region.
>>>>>>>
>>>>>>>
>>>>>>> Index: itkImageConstIterator.h
>>>>>>> ===================================================================
>>>>>>> RCS file: /cvsroot/Insight/Insight/Code/Common/
>>>>>>> itkImageConstIterator.h,v
>>>>>>> retrieving revision 1.23
>>>>>>> diff -r1.23 itkImageConstIterator.h
>>>>>>> 175a176,183
>>>>>>>
>>>>>>>
>>>>>>>> const RegionType & bufferedRegion = m_Image->GetBufferedRegion();
>>>>>>>
>>>>>>>> if( ! bufferedRegion.IsInside( m_Region ) )
>>>>>>>
>>>>>>>>  {
>>>>>>>
>>>>>>>>  itkGenericExceptionMacro("Region " << m_Region
>>>>>>>
>>>>>>>>    << " is outside of buffered region " << bufferedRegion );
>>>>>>>
>>>>>>>>  }
>>>>>>>
>>>>>>> Index: itkImageConstIteratorWithIndex.txx
>>>>>>> ===================================================================
>>>>>>> RCS file: /cvsroot/Insight/Insight/Code/Common/
>>>>>>> itkImageConstIteratorWithIndex.txx,v
>>>>>>> retrieving revision 1.27
>>>>>>> diff -r1.27 itkImageConstIteratorWithIndex.txx
>>>>>>> 84a85,92
>>>>>>>
>>>>>>>
>>>>>>>> const RegionType & bufferedRegion = m_Image->GetBufferedRegion();
>>>>>>>
>>>>>>>> if( ! bufferedRegion.IsInside( m_Region ) )
>>>>>>>
>>>>>>>> {
>>>>>>>
>>>>>>>> itkGenericExceptionMacro("Region " << m_Region
>>>>>>>
>>>>>>>>  << " is outside of buffered region " << bufferedRegion );
>>>>>>>
>>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> Does anyone see a problem with this patch ?
>>>>>>>
>>>>>>> Any objections ?
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>
>>>>>>>    Luis
>>>>>>> _______________________________________________
>>>>>>> Powered by www.kitware.com <http://www.kitware.com>
>>>>>>>
>>>>>>> Visit other Kitware open-source projects at
>>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>>
>>>>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>>>>
>>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>
>>>>>> ========================================================
>>>>>> Bradley Lowekamp  Lockheed Martin Contractor for
>>>>>> Office of High Performance Computing and Communications
>>>>>> National Library of Medicine blowekamp at mail.nih.gov
>>>>>> <mailto:blowekamp at mail.nih.gov
>>>>>
>>>>> _______________________________________________
>>>>> Powered by www.kitware.com
>>>>>
>>>>> Visit other Kitware open-source projects at
>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>
>>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>>
>>>>> Follow this link to subscribe/unsubscribe:
>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>
>>>> _______________________________________________
>>>> Powered by www.kitware.com
>>>>
>>>> Visit other Kitware open-source projects at
>>>> http://www.kitware.com/opensource/opensource.html
>>>>
>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>
>>>> Follow this link to subscribe/unsubscribe:
>>>> http://www.itk.org/mailman/listinfo/insight-developers
>
> --
> Gaëtan Lehmann
> Biologie du Développement et de la Reproduction
> INRA de Jouy-en-Josas (France)
> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
> http://voxel.jouy.inra.fr  http://www.mandriva.org
> http://www.itk.org  http://www.clavier-dvorak.org
>
>

More information about the Insight-developers mailing list