[Insight-developers] ImageRegionIterator consistency issues

[Zachary Pincus]

Bug 2781 filed, fixed in the CVS, and closed to resolve the two copy- 
constructors that do not compile. I will defer on the larger issue of  
what to do about the copy constructors in general.

The "API" whereby an ImageIteratorWithIndex could be copy-constructed  
to an ImageRegionIterator was removed, but since there is no way that  
this code could have ever compiled, much less been successfully used,  
I don't think that counts as a deprecation of anything. It has been  
replaced with a copy constructor for creating ImageRegionIterator  
from an ImageIterator, which is what the intention of the original  
code (modulo putative copy-paste errors) was likely to have been.

Zach


On Jan 27, 2006, at 12:27 PM, Zachary Pincus wrote:

>> 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
>
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers