[Insight-developers] ImageRegionIterator consistency issues

[Zachary Pincus]

> Given these conversions are probably not used within ITK proper, one
> can make an argument to remove them.  However, we have to be  
> consignant
> of backward compatibility issues. Someone, somewhere, might be using
> one of these conversions.

I suspect that at least the conversion from ImageIterator to  
ImageRegionConstIterator and from ImageIteratorWithIndex to  
ImageRegionIterator are not in use anywhere because they do not  
compile properly.

There actually don't seem to be any other defined iterator  
conversions across hierarchies (e.g. from a WithIndex to a normal  
iterator). I suspect that the ImageIteratorWithIndex ->  
ImageRegionIterator conversion was a cut-and-paste error or similar.

I then suggest amending the policy to only copy iterators within a  
single hierarchy. This means then that each iterator needs to provide  
a copy constructor that can accept its base class. No need for cross- 
copy between WithIndex and regular iterators, unless someone actually  
wants to do all of that work.

The other option, which requires less work, is to deprecate the  
entire copy constructor API. I don't suggest removing what's there  
now, but maybe not making the effort to extend the API to the rest of  
the (now many) iterator types will be OK. (I think this may be a "who  
wants to bell the cat" issue.)

At any rate, unless otherwise directed, I will remove the  
ImageIteratorWithIndex -> ImageRegionIterator conversion from  
ImageRegionIterator because (a) it doesn't make sense, and (b) it  
does not, and has probably never, compiled propely. I'll also commit  
the fix to the ImageRegionConstIterator that I initially proposed so  
that that one compiles properly.

Zach


> Your proposed policy looks reasonable.  I would limit it to  
> ImageIterator
> and ImageIteratorWithIndex hierarchies. Probably need to define  
> whether
> copying an iterator also copies its current position within the  
> region.
>
> Jim
>
>
>
> -----Original Message-----
> From: insight-developers-bounces+millerjv=crd.ge.com at itk.org
> [mailto:insight-developers-bounces+millerjv=crd.ge.com at itk.org]On  
> Behalf
> Of Zachary Pincus
> Sent: Thursday, January 26, 2006 5:24 PM
> To: insight-developers at itk.org
> Subject: Re: [Insight-developers] ImageRegionIterator consistency  
> issues
>
>
> On closer inspection, it seems like the way in which image iterators
> are interconverted in ITK could use a review.
>
> The documentation states "Many routines return an ImageIterator but
> for a particular task, you may want an ImageRegionIterator.  Rather
> than provide overloaded APIs that return different types of
> Iterators, itk returns ImageIterators and uses constructors to cast
> from an ImageIterator to a ImageRegionIterator" (and so forth for
> other iterator subclasses).
>
> The problem is that the implementation of this is patchwork at best.
>
> Specifically, there are multiple base classes for image iterators:
> ImageConstIterator, ImageConstIteratorWithIndex, and
> ConstNeighborhoodIterator (at least). Assuming that conversion
> between the first two and the last is uncommon, this means that there
> need to be methods to convert an ImageConstIterator to an
> ImageConstIteratorWithIndex, and vice-versa.
>
> In fact, ImageRegionIterator has a constructor which claims to accept
> an ImageIteratorWithIndex. But of course there's no method to
> accomplish this, so that function would fail to compile if it were
> ever used.
>
> Why this constructor is defined in ImageRegionIterator and nowhere
> else, and why it takes an ImageIteratorWithIndex and not the
> ConstIterator version, and so forth, are of course completely
> mysterious.
>
> Perhaps defining a more strict policy about how iterators need to be
> converted would be best? Otherwise perhaps the whole bit ought to be
> deprecated (given that much of this API fails even to compile, I
> doubt anyone's using it), and people who need to convert between
> iterator types will have to do so manually, depending on their
> specific need?
>
> Otherwise the policy would look something like this:
> (1) Iterator base classes will implement constructor methods to
> accept iterators of other base class types.
>
> (2) Other iterator classes will implement constructors which accept
> each iterator base class type (which call the superclass constructor
> methods, plus do any additional calculations necessary). Doing things
> via calling superclass operator= methods, as done currently, seems
> marginally suspect.
>
> This will ensure that basic iterator conversion works. There could be
> other constructors defined to allow more precise conversions (e.g.
> ImageRegionConstIterator <-> ImageRegionConstIteratorWithIndex
> without losing the region information that would happen if the
> conversion went through the non-region-enabled superclasses.)
>
> But this all seems like more work than may be worth while, given the
> fact that this API is partially unused (and partially untested, and
> partially  doesn't compile).
>
> Zach
>
>
> On Jan 26, 2006, at 2:02 AM, Zachary Pincus wrote:
>
>> Hello again -
>>
>> Today Gaetan and I have uncovered some minor consistency issues in
>> the ImageRegionIterators. One issue I can fix (and will in the next
>> few days); one I am not sure of what to do about.
>>
>> --------
>> Issue the first:
>>
>> itkImageRegionConstIterator.h defines a constructor that looks like
>> this:
>>   ImageRegionConstIterator( const ImageIterator<TImage> &it)
>>   {
>>     this->ImageIterator<TImage>::operator=(it);
>>     ...
>>   }
>>
>> The only problem is that ImageRegionConstIterator doesn't inherit
>> from ImageIterator, so the first line of the constructor doesn't
>> compile. Since this iterator inherits from ImageConstIterator, the
>> first line should look like this:
>>     this->ImageConstIterator<TImage>::operator=(it);
>>
>> This should fix things.
>>
>> --------
>> Issue the second:
>>
>> itkImageRegionIterator.txx defines a constructor that looks like  
>> this:
>>
>> template< typename TImage >
>> ImageRegionIterator<TImage>
>> ::ImageRegionIterator( const ImageIteratorWithIndex<TImage> &it):
>>   ImageRegionConstIterator<TImage>(it)
>> {
>> }
>>
>> Unfortunately, ImageRegionConstIterator doesn't define any
>> constructors that are compatible with an ImageIteratorWithIndex
>> parameter, so this method also fails to compile. I'm not sure what
>> the correct approach would be for fixing this issue.
>>
>> Zach
>>
>> _______________________________________________
>> Insight-developers mailing list
>> Insight-developers at itk.org
>> http://www.itk.org/mailman/listinfo/insight-developers
>
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers