ITK icon indicating copy to clipboard operation
ITK copied to clipboard

STYLE: Remove unused "itkFrequencyImageRegion*IteratorWithIndex.h" files

Open N-Dekker opened this issue 1 year ago • 4 comments

Both FrequencyImageRegionIteratorWithIndex and FrequencyImageRegionConstIteratorWithIndex appear unused and untested. Attempts to use them would trigger compile errors.

An attempt to use FrequencyImageRegionIteratorWithIndex triggered:

error C3210: 'ImageRegionIteratorWithIndex<itk::Image<int,2> >': a member using-declaration can only be applied to a base class member

An attempt to use FrequencyImageRegionConstIteratorWithIndex triggered:

error C2679: binary '=': no operator found which takes a right-hand operand of type 'const itk::Point<T,3>' (or there is no acceptable conversion)

This commit removes their header files. It also removes the suggestion to use FrequencyImageRegionIteratorWithIndex from the documentation of UnaryFrequencyDomainFilter.

N-Dekker avatar Aug 29 '24 09:08 N-Dekker

@phcerdan Sorry Pablo 🤷 but if those iterators don't compile and don't have any test, I think it's better not to have them in the main branch!

N-Dekker avatar Aug 29 '24 09:08 N-Dekker

@phcerdan Thanks for your very quick response, Pablo!

They are used in the IsotropicWavelets module. We should move the tests from there here.

Sorry, but how can they possibly be used when they cannot compile at all? Even the very first revision of those two iterators (six years ago, e16d0bc4eec7b3ee7ab63c3469077d54390df5c6) would have those compile errors, as far as I can see.

The other six iterator types from Modules/Filtering/ImageFrequency compile fine, it's just these two. So I suspect that these two are not really necessary. 🤷

N-Dekker avatar Aug 29 '24 11:08 N-Dekker

Thanks @N-Dekker, I am out of office and away from keyboard to give a detailed answer about the history of the filters. But if you don't want to work on a fix, let me do it when I am back.

Thanks

phcerdan avatar Aug 29 '24 11:08 phcerdan

But if you don't want to work on a fix, let me do it when I am back.

OK, thanks Pablo. Rather than just a quick-fix of those compile errors, I hope you can add a test that will show the usefulness of those two iterators.

  • By the way, I encountered those compile errors when I was busy writing a unit test for pull request #4827, testing that Iterator(image, region) yields an iterator at the begin of the region. This test passed well for the other six ImageFrequency iterator types. Please have a look, if you still have time!

N-Dekker avatar Aug 29 '24 12:08 N-Dekker