Skip to content

Conversation

@phcerdan
Copy link
Contributor

Test it via recently added gtest (see #4827)

Closes #4828

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module labels Sep 10, 2024
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

LGTM

@N-Dekker
Copy link
Contributor

Thanks @phcerdan but are those two iterators really useful? Given the fact that these two did not compile in the six years of their existence, why not remove them by now?

If they are really useful (next to all the other iterator types that we have already in ITK), can you please add a test or example to show their usefulness?

@phcerdan
Copy link
Contributor Author

phcerdan commented Sep 11, 2024

Hi @N-Dekker, ITK is a library, not an application, we provide filters that are useful for others. This iterator in particular is a really simple extension of the image iterator to provide the GetFrequency() functions that are used for FrenquencyFunctors.

This iterator in particular captures the rare (but existing, I have worked with those images from diffusion experiments in optics) where the input image is already in the frequency domain.
The more common case is to have an image in the spatial domain and perform an FFT, that is captured by the other frequency iterators.

It is here for completion on the frequency iterators, and ability to work with this kind of images.

The frequency iterators have all the same interface, and it is tested, I don't think it is necessary to add an extra test. beyond the instantiation test that is already in place, for this one, which is the simplest.

@N-Dekker
Copy link
Contributor

@phcerdan OK, thanks for your explanation. Then I just hope that these two iterators are really going to be used! In general I believe it's a good thing to regularly clean-up the library, and remove unused components.

Can you please still run clang-format, and amend (+ force-push) your commit?

@phcerdan phcerdan force-pushed the fix_itk_region_frequency_iterator branch from 32b5990 to a1a1fce Compare September 11, 2024 13:44
@phcerdan
Copy link
Contributor Author

Done

@phcerdan phcerdan merged commit ab16ca6 into InsightSoftwareConsortium:master Sep 11, 2024
@phcerdan phcerdan deleted the fix_itk_region_frequency_iterator branch September 11, 2024 13:45
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:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants