[Insight-developers] ImageIterators : Suggested Patch: Exception

Luis Ibanez luis.ibanez at kitware.com
Sat May 9 15:19:33 EDT 2009 

Gaetan,

Excellent,

Thanks for taking care of that problem.


    Luis


-----------------------
Gaëtan Lehmann wrote:
> 
> Luis,
> 
> I think it is fixed now, for the morphology part.
> 
> Gaëtan
> 
> 
> Le 8 mai 09 à 14:13, Luis Ibanez a écrit :
> 
>> 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