[Insight-developers] ImageIterators : Suggested Patch: Exception

[Gaëtan Lehmann]

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.

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

-- 
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/20090505/f2b3ab4a/attachment.pgp>