[Insight-developers] ImageIterators : Suggested Patch: Exception
Gaëtan Lehmann
gaetan.lehmann at jouy.inra.fr
Fri May 8 18:37:21 EDT 2009
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
>>
>>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: Ceci est une signature ?lectronique PGP
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090509/310da264/attachment.pgp>
More information about the Insight-developers
mailing list