[Insight-developers] ImageRegionIterator consistency issues

[Miller, James V (GE, Research)]

Zachary, 

As you reference from the documentation, the intention of the these
constructors was to allow iterators to be converted from one type
to another.  Since this original design, the following iterators 
have been added to the system:

* All the const versions of the iterators
* All the WithIndex style of iterators
* (and the Neighborhood and FloodFill iterators)

This aspect of the iterator design just hasn't kept pace with the new
iterator types introduced with the toolkit.

I am not sure how the frequently these iterator conversions are used.
The design originally allowed for a routine to simple return and ImageIterator
and the subclasses of ImageIterator could convert to the appropriate 
subclass. By this, one could construct an image iterator subclass by 
getting the Image pointer and ImageRegion from the parameter ImageIterator.

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.

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