[Insight-developers] ImageIterators : Suggested Patch: Exception

[Karthik Krishnan]

Brad:

We don't have any "guideline" for preconditions. We have compile time
preconditions (also known as ConceptChecking in ITK). Run time preconditions
are flagged by throwing exceptions and the guideline is that the class
throws an exception giving an informative message to that effect.

But I agree with Brad that we may need a guideline, if we start checking for
too many preconditions in a way that may impact performance. For instance
assert is exercised only in Debug builds. One could use the assert itself to
use some of these preconditions or somehow define a macro using
ITK_LEAN_AND_MEAN to conditionally exercise these checks.

In this specific case, the performance impact is negligible.

Thanks
--
karthik

On Tue, May 5, 2009 at 12:14 PM, Luis Ibanez <luis.ibanez at kitware.com>wrote:

>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090505/8310c474/attachment.htm>