Of course, I very much encourage using the new iterator ranges, introduced with ITK 5 (ImageBufferRange, ImageRegionRange, ShapedImageNeighborhoodRange, IndexRange). But I have to admit that “traditional” ITK iterators like ImageRegionIterator and ImageRegionIteratorWithIndex have an extra feature: while iterating over a region, they allow easily accessing each pixel and retrieving its N-dimensional index, during the very same iteration step.
Unfortunately, there is a caveat there: while iterator.GetIndex() is very fast for an ImageRegionIteratorWithIndex, it is relatively slow for an ImageRegionIterator. ImageRegionIterator is derived from ImageConstIterator, which implements GetIndex() by doing m_Image->ComputeIndex(m_Offset). That can be very time consuming, especially when it is performed iteratively, for each pixel.
I believe that it’s too easy to mistakenly assume that iterator.GetIndex() is fast when it is not, and make performance bugs. And indeed, I see quite a few cases in the ITK source code where iterator.GetIndex() is called multiple times on the same location, and where iterator.GetIndex()is called iteratively, for each pixel, even while it’s doing a potentially time consuming computation to compute the index. So that’s why I’m proposing a new member function, ImageConstIterator::ComputeIndex(), which is equivalent to the old ImageConstIterator::GetIndex() , but much clearer with respect to its performance cost:
I believe that using iterator.ComputeIndex()instead of iterator.GetIndex()will very much ease finding the aforementioned performance bugs.
Eventually the old ImageConstIterator::GetIndex() may then become deprecated, but I think we should still support the old member function for quite some time, as there is still a lot of legacy user code depending on this member function. What do you think?
I agree! I was not aware of this limitation and would find it useful that my code stops compiling with (ITK_LEGACY_REMOVE=TRUE at first) when this is adopted. You should make sure that the motivation described here is easily found by the user when the problem occurs, possibly with a link to this discourse message when ITK_LEGACY_REMOVE=TRUE and a compilation error occurs.
It seems that you haven’t prepared for removing it in the existing commit, why not already putting ImageConstIterator::GetIndex() in legacy preprocessor directives? That would make sense to me as there is a major release upcoming.
Eventually I think it’s preferable to do as you’re suggesting: deprecate the old computative GetIndex() from ImageConstIterator, declaring it legacy-only. I just don’t want to over-hurry, as it would be a breaking change, and we are already close to releasing ITK 6. Simply adding an alternative, ComputeIndex(), would already be a step forward, without breaking any user code.
On the other hand, if there appears sufficient time to do so with regard to ITK’s release schedule, I would be fine with declaring ImageConstIterator::GetIndex() “future legacy remove” (ITK_FUTURE_LEGACY_REMOVE).
The expected time line for a function that is marked FUTURE LEGACY REMOVE now is to be just LEGACY remove in 7, and then COMPATIBILITY in 8, then actually removed in 9. I would not classify that as being removed in a “hurry”.