[Insight-developers] ImageIterators : Suggested Patch: Exception

Luis Ibanez luis.ibanez at kitware.com
Fri May 8 11:28:29 EDT 2009 

Hi Gaetan,

BTW, I forgot to mention that an initial implementation
      of the code for checking regions in Iterator constructors
      was committed on Wednesday.

You can enable it by turning ON the new CMake variable:


       ITK_USE_REGION_VALIDATION_IN_ITERATORS


When this variable is ON, Iterators with throw exceptions if
they are called with a region that is not fully included in
the image Buffered Region.


    Regards,


       Luis


----------------------
Gaëtan Lehmann wrote:
> 
> 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
> 
>

More information about the Insight-developers mailing list