PERF: unsharpenedImage iterator in N4Bias...Filter is now "WithIndex"

[N-Dekker]

Declared the unsharpenedImage iterator within N4BiasFieldCorrectionImageFilter::SharpenImage as ImageRegionConstIteratorWithIndex, instead of ImageRegionConstIterator.

Observed a significant performance improvement: 10% to 30% reduction of runtime duration on filter->Update().

[N-Dekker]

@ntustison Please review! It's just one line of code :-)

[hjmjohnson]

@N-Dekker Do you have an intuition why this is faster? I thought that the "WithIndex" was the slower iterator.

[blowekamp]

I had similar experience last I profiled ImageRegion iterators. I would also recommend comparing the performance with the ImageScanelineConstIterator. I designed that iterator to help enable automatic vectorization of the inner loops. When I profiled it it provides a consistent performance improvements across different compilers.

[N-Dekker]

@hjmjohnson wrote:

@N-Dekker Do you have an intuition why this is faster? I thought that the "WithIndex" was the slower iterator.

Good question! A "WithIndex" iterator type appears much, much faster than the corresponding iterator type "without index" when calling it.GetIndex(). As is done within the for-loop at:

ITK/Modules/Filtering/BiasCorrection/include/itkN4BiasFieldCorrectionImageFilter.hxx

Line 252 in ddcc43a

( useMaskLabel && maskImage->GetPixel( ItU.GetIndex() ) == maskLabel )

and

ITK/Modules/Filtering/BiasCorrection/include/itkN4BiasFieldCorrectionImageFilter.hxx

Line 256 in ddcc43a

confidenceImage->GetPixel( ItU.GetIndex() ) > 0.0 ) )

I'm not an expert on the old ITK image iterators. But I just saw that itkN4BiasFieldCorrectionImageFilter.hxx already uses "WithIndex" iterators in similar for-loops, so it seems consistent to also use it for unsharpenedImage. https://github.com/InsightSoftwareConsortium/ITK/blob/v5.0b01/Modules/Filtering/BiasCorrection/include/itkN4BiasFieldCorrectionImageFilter.hxx And I did observe a major performance improvement, with this very tiny patch.

@blowekamp Thanks for your suggestion. If you think itk::ImageScanlineConstIterator is preferable in this case, you may adapt the code accordingly, of course!

[hjmjohnson]

OH! That makes perfect sense to me then. I wonder if we should remove (or deprecate, or warn) about using the GetIndex from "ImageRegionConstIterator".

My naive assumption was that ImageRegionConstIterator did NOT provide a GetIndex method, and that you needed to use ImageRegionConstIteratorWithIndex to have a GetIndex method..

[ntustison]

@N-Dekker It's fine with me but I'd defer to @hjmjohnson and @blowekamp for these types of code considerations.

[N-Dekker]

@ntustison Thanks for your reply. I find your filter very interesting. Unfortunately it's quite time consuming, at least for our MRI data set. One more question: is it OK to assume that the optional mask image and the optional confidence image always have the same dimensions (same number of rows, columns, and slices) as the input image? This assumption could possibly be used to further speed up the filter.

[ntustison]

@N-Dekker Normally, N4 isn't applied to the full resolution image. You can see this from our default parameters in the ANTs N4 program. This is also true of the original N3. Regardless, as to your question:

One more question: is it OK to assume that the optional mask image and the optional confidence image always have the same dimensions (same number of rows, columns, and slices) as the input image?

Yes.

[N-Dekker]

@ntustison Thanks again for you reply. Thanks for confirming my assumption that the optional mask image and the optional confidence image have the same size as the input image.

@hjmjohnson, @blowekamp I do have some more ideas on how to possibly further improve the performance, but I need to double-check. If the filter processes the entire image data at once (as I understand from the calls to GetLargestPossibleRegion(), in itkN4BiasFieldCorrectionImageFilter.hxx), I think it's not needed to use N-dimensional indices in most cases, when querying pixel values from input image, mask image, and confidence image....

...However, for now I'd be happy if this very tiny patch could be accepted! It seems like low-hanging fruit :-)

[hjmjohnson]

@N-Dekker Thank you! Speed improvements in the often used filter would be greatly appreciated!

[N-Dekker]

@hjmjohnson wrote:

I wonder if we should remove (or deprecate, or warn) about using the GetIndex from "ImageRegionConstIterator".

It's somewhat confusing that an ITK iterator without "WithIndex" still has a GetIndex() member function, which might suggest that it would simply return some m_Index data member. Although it is documented correctly that this member function is "an expensive operation" of ImageConstIterator:
https://itk.org/Doxygen/html/classitk_1_1ImageConstIterator.html#aae11801c25a113360119ef69ec62b1d0

The functionality itself still seems quite useful to me, so I'm not sure if it should be removed. But it might be more clear if the ImageConstIterator member function would be renamed to something like CalculateIndex()... right?

At the moment, it's not my highest priority, but if you would like to rename the member function, please do!

[blowekamp]

While this is with the ScanlineIterator and not the ImageRegion, it does provide an example of how the expensive GetIndex method can be used responsible just a coupe times at the boundaries of a region, and not at every pixel:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Filtering/ImageGrid/include/itkResampleImageFilter.hxx#L445

These are the tricks of the trade want you want things to go fast in ITK.