Skip to content

ENH: Expose FFT filter dimension traits#3152

Merged
tbirdso merged 1 commit intoInsightSoftwareConsortium:masterfrom
tbirdso:expose-fft-factory-dimension-list
Feb 3, 2022
Merged

ENH: Expose FFT filter dimension traits#3152
tbirdso merged 1 commit intoInsightSoftwareConsortium:masterfrom
tbirdso:expose-fft-factory-dimension-list

Conversation

@tbirdso
Copy link
Contributor

@tbirdso tbirdso commented Feb 2, 2022

Moves FFT filter image dimensions into existing trait objects in order
to decouple FFT factory overrides from fixed dimension lists. This will
allow other, external FFT implementations that may not be valid for
higher-dimension overrides to reuse the FFT factory class.

Future changes may replace the explicit dimension lists with CMake
variables to avoid registering unnecessary factories.

Motivation for these changes comes from developing accelerated FFT filter overrides in the ITKVkFFTBackend external module.

PR Checklist

Refer to the ITK Software Guide for
further development details if necessary.

Moves FFT filter image dimensions into existing trait objects in order
to decouple FFT factory overrides from fixed dimension lists. This will
allow other, external FFT implementations that may not be valid for
higher-dimension overrides to reuse the FFT factory class.

Future changes may replace the explicit dimension lists with CMake
variables to avoid registering unnecessary factories.
@github-actions github-actions bot added area:Filtering Issues affecting the Filtering module type:Enhancement Improvement of existing methods or implementation labels Feb 2, 2022
@blowekamp
Copy link
Member

Looks like a good step forward. Do you see the CMake options being a list like this or a start, stop range?

It is a shame that the std::make_integer_sequence starts at 0, so we can't directly use it. The results of it could have additional performed on each integer element with a variadic template to perform addition. I could write a class to do that if you think it would help with the final cmake parameters needed.

@tbirdso
Copy link
Contributor Author

tbirdso commented Feb 2, 2022

@blowekamp I haven't tried it yet but I think the simplest way would be to rely on the explicit list of dimensions specified by the user in ITK_WRAP_IMAGE_DIMS, or define a similar CMake variable to do the same. That way the logic for creating dimension lists would be handled in the CMake config step and the code you've already written (thanks!) for unpacking the integer_sequence would suffice for handling overrides at compile time.

@Leengit
Copy link
Contributor

Leengit commented Feb 2, 2022

I didn't dive deeply enough to understand these variadic templates ... apologies if this is a dumb question:

In
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Filtering/FFT/include/itkFFTImageFilterFactory.h#L98
and
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Filtering/FFT/include/itkFFTImageFilterFactory.h#L108
what is the advantage of having the likes of

const std::integer_sequence<unsigned int, D, ImageDimensions...> &

as an argument to the method? Could the expression that appears as an argument be eliminated on line 98 making this a function of void? On line #108, could the type that appears as an argument be placed with the other template parameters rather than as a function argument?

I am thinking that the usual arguments about partially templated methods do not apply. Is it a matter of style / preference?

@tbirdso
Copy link
Contributor Author

tbirdso commented Feb 2, 2022

@Leengit It looks like there was discussion around this in #3065 regarding how the recursive procedure is terminated, to me this falls under an "if it ain't broke" scenario. If you and @blowekamp decide that a different implementation would be advantageous it may be best to track discussion in a dedicated issue.

@Leengit
Copy link
Contributor

Leengit commented Feb 2, 2022

how the recursive procedure is terminated

Of course. That mine was a dumb question is now confirmed. :-)

@tbirdso
Copy link
Contributor Author

tbirdso commented Feb 3, 2022

@Leengit Not a dumb question, thanks for double-checking on something that wasn't immediately clear. If/when ITK moves to C++17 one day in the future, we could replace the current recursive implementation with something like a fold expression to greatly improve readability.

@tbirdso tbirdso merged commit ddef776 into InsightSoftwareConsortium:master Feb 3, 2022
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:Enhancement Improvement of existing methods or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants