Skip to content

Conversation

@N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Aug 29, 2024

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.

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 N-Dekker requested a review from phcerdan August 29, 2024 09:04
@github-actions github-actions bot added area:Filtering Issues affecting the Filtering module type:Style Style changes: no logic impact (indentation, comments, naming) labels Aug 29, 2024
@N-Dekker
Copy link
Contributor Author

@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!

Copy link
Contributor

@phcerdan phcerdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

These filters are useful, prefer a fix rather than removal.

@N-Dekker
Copy link
Contributor Author

@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, e16d0bc) 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. 🤷

@phcerdan
Copy link
Contributor

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

@N-Dekker
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Filtering Issues affecting the Filtering module type:Style Style changes: no logic impact (indentation, comments, naming)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants