STYLE: Use recursive FFT registration for N to 1

[blowekamp]

Instead of manually listing the FFT filter dimensions to regisiter,
use a recursive function templated over an integer. This in the future
can easily enable the maximum FFT dimension to be registered as some
type of configuration parameter.

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)
• Added license to new files (if any)
• Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• Added ITK examples for all new major features (if any)

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

[Leengit]

How does the approach

  template <typename InputPixelType, typename OutputPixelType, unsigned int ImageDimension>
  void
  OverrideFFTImageFilterType(const std::integral_constant<unsigned int, ImageDimension> &)
  {
  // ...
  }
  // Invoke with
  OverrideFFTImageFilterType<InputPixelType, OutputPixelType>(std::integral_constant<unsigned int, D>());

compare with

  template <typename InputPixelType, typename OutputPixelType, unsigned int ImageDimension>
  void
  OverrideFFTImageFilterType()
  {
  // ...
  }
  // Invoke with
  OverrideFFTImageFilterType<InputPixelType, OutputPixelType, D>();

[blowekamp]

@Leengit The approach taker here also does the following to "specialized" the use when the dimension is 0:

template <typename InputPixelType, typename OutputPixelType>
   void
   OverrideFFTImageFilterType(const std::integral_constant<unsigned int, 0> &)
   {}

As you can not do partial template specialization for member function, the approach in this patch uses member function overload and depends on the C++ specification preferring the overloaded method that is more specific. ( Sorry know know the specific term. ).

With the proposed alternative how is the template parameter recursion terminated?

[N-Dekker]

@blowekamp Interesting!

I think it would be nice if std::make_integer_sequence<unsigned, N> could be uses here, as it creates a compile-time sequence of numbers 0 to N-1.

[Leengit]

can not do partial template specialization

Yes, I understand now. Although I venture that we could emulate the partial template specialization for the method, with std::enable_if and SFINAE, I like your approach.

[blowekamp]

it would be nice if std::make_integer_sequence<unsigned, N> could be uses here, as it creates a compile-time sequence of numbers 0 to N-1.

@N-Dekker That looks "nifty". I'm not sure that the usage would be more compact, clearer or offer any benefit over using new C++/STL features.

[blowekamp]

Yes, I understand now. Although I venture that we could emulate the partial template specialization for the method, with std::enable_if and SFINAE, I like your approach.

I have generally found std::enable_if give a little more clutter and complication to functions. The simpler overloaded approach is less code and is easier to understand.

[tbirdso]

@blowekamp Thanks for working on this, very good idea to move from manually setting dimensions to a more configurable pattern.

I'm wondering if there is room here to support class overrides not necessarily starting at Dimension = 1 for extensibility.
Following up from @N-Dekker 's comment, what are your thoughts on using std::index_sequence to expose the dimension override list to the developer, with a fallback to std::make_index_sequence<4>()?

I haven't tried compiling but I imagine something exposing override dimensions through the FFTImageFilterTraits struct like this:

template <template <typename, typename> class TFFTImageFilter>
struct FFTImageFilterTraits
{
  using DimensionList = std::make_index_sequence<4>;
};


template <template <typename, typename> class TFFTImageFilter>
class FFTImageFilterFactory : public itk::ObjectFactoryBase
{
...
  FFTImageFilterFactory()
  {
    (OverrideFFTImageFilterType<typename FFTImageFilterTraits<TFFTImageFilter>::template InputPixelType<float>,
                                   typename FFTImageFilterTraits<TFFTImageFilter>::template OutputPixelType<float>,
                                   FFTImageFilterTraits<TFFTImageFilter>::DimensionList>(), ...);
...
}

If pursuing this would pose an undue burden, though, I'm fine with moving ahead as-is and revisiting if a need arises at some point in the future.

EDIT: I researched this a bit more and fold expressions were introduced in C++17. We are targeting C++14, which I believe would make the block suggested here in FFTImageFilterFactory() much more messy and less than ideal to implement.

[dzenanz]

It looks like we are all in agreement: this is an improvement. Planning to merge later today, unless someone objects or beats me to it 😄

[blowekamp]

Well I gave into peer pressure and updated to the std::integer_sequence thing... Please wait to the CI to finished.

[N-Dekker]

@blowekamp I think the advantage of std::integer_sequence should be that C++ users then no longer need to do those recursive function calls themselves. But now I see you added std::integer_sequence, and you still have the recursive function call!!! Doesn't that beat the purpose?

[dzenanz]

Wasn't the purpose to allow skipping some numbers?

[blowekamp]

@blowekamp I think the advantage of std::integer_sequence should be that C++ users then no longer need to do those recursive function calls themselves. But now I see you added std::integer_sequence, and you still have the recursive function call!!! Doesn't that beat the purpose?

I'm not sure what you had in mind then. You should give a go at implementing as you'd like 😉

[tbirdso]

I agree with @dzenanz, the implementation shouldn't matter so much as exposing functionality to external developers. @blowekamp 's implementation sets up a good structure to allow wrapping dimensions to be explicitly specified rather than always being in the range from 1 to N. Its recursive nature is not exposed to the developer and isn't too difficult to read here.

I will point out that while the current changes alter the structure of FFTImageFilterFactory(), they don't actually change the interface as seen by the external developer, i.e. no mechanism is currently introduced to allow FFT classes to be wrapped for different dimensions than the specified 4,3,2,1. I touched on it in my earlier comment, but we could expose that configuration by adding a field to the existing FFTImageFilterTraits object which is later specialized for each overriding class:

template <template <typename, typename> class TFFTImageFilter>
struct FFTImageFilterTraits
{
  using DimensionList = std::make_integer_sequence<unsigned int, 4>;
};
...
OverrideFFTImageFilterType<typename FFTImageFilterTraits<TFFTImageFilter>::template InputPixelType<float>,
  typename FFTImageFilterTraits<TFFTImageFilter>::template OutputPixelType<double>>(
  FFTImageFilterTraits<TFFTImageFilter>::DimensionList);

@blowekamp it's up to you whether this is within the scope of this PR to investigate or not.

[blowekamp]

it's up to you whether this is within the scope of this PR to investigate or not.
Thanks for the kind explanation.

I did give this a try, but since all the specialized FFTImageFilterTraits classes don't inherit from the one in this file, all the traits would need to be updated. I passed on the one, but you are welcome to take over the PR to get that last step.

With this proposed addition, it not clear how this could be configurable. For example ITK Python was configured with 5d could it be enabled some how? Or SimpleITK does not use 4D ffts, is there a way to disable that from being compiled. ( I think the 4D implementation may be in the libraries too, adding to the library bulk.... I should check. )

[tbirdso]

I did give this a try, but since all the specialized FFTImageFilterTraits classes don't inherit from the one in this file, all the traits would need to be updated. I passed on the one, but you are welcome to take over the PR to get that last step.

Ah good point, thanks for giving it a shot. I think these changes stand alone without it, I can investigate and submit a subsequent PR down the line.

With this proposed addition, it not clear how this could be configurable. For example ITK Python was configured with 5d could it be enabled some how? Or SimpleITK does not use 4D ffts, is there a way to disable that from being compiled. ( I think the 4D implementation may be in the libraries too, adding to the library bulk.... I should check. )

You're right, I meant "configurable" as opposed to manually outlining a factory registration call for each distinct dimension. In my mind the optimal approach for Python-wrapped files would be to only register default factories against the image values outlined in ITK_WRAP_IMAGE_DIMS and below, as you're saying it could be useful to introduce an additional CMake variable to allow the user to explicitly limit default factory dimensions at compile time. However this would require introducing a new .in file for configuration and would require nontrivial discussion and development outside of my bandwidth for the moment.

[dzenanz]

Is this ready for merging now?

[N-Dekker]

Could OverrideFFTImageFilterType still be declared private (instead of protected)? It would give us more freedom to do further adjustments in the future, if necessary. (protected members are part of the interface towards derived classes.)

[N-Dekker]

It appears that OverrideFFTImageFilterType could be written without recursion, approximately as follows:

  template <typename TInputPixel, typename TOutputPixel, unsigned int VImageDimension>
  using FilterAlias = TFFTImageFilter<Image<TInputPixel, VImageDimension>, Image<TOutputPixel, VImageDimension>>;

  template <typename InputPixelType, typename OutputPixelType, unsigned int... ImageDimensions>
  void
  OverrideFFTImageFilterType(const std::integer_sequence<unsigned int, ImageDimensions...> &)
  {
    (void)(std::initializer_list<int>{
      (this->RegisterOverride(
         typeid(typename FilterAlias<InputPixelType, OutputPixelType, ImageDimensions>::Superclass).name(),
         typeid(FilterAlias<InputPixelType, OutputPixelType, ImageDimensions>).name(),
         "FFT Image Filter Override",
         true,
         CreateObjectFunction<FilterAlias<InputPixelType, OutputPixelType, ImageDimensions>>::New()),
       0)... });
  }

Using the wonderful (void)(std::initializer_list<int>{ (... syntax as in the print_tuple_impl(const std::tuple<Ts...>&, std::index_sequence<I...>) code example by Matt Aubury, from an article that is interestingly named RIP index_sequence, 2014–2017

Unfortunately, VS2019 says:

\ITK\Modules\Filtering\FFT\include\itkFFTImageFilterFactory.h(109,1): fatal error C1001: Internal compiler error.
(compiler file 'msc1.cpp', line 1603)
To work around this problem, try simplifying or changing the program near the locations listed above.
If possible please provide a repro here: https://developercommunity.visualstudio.com
Please choose the Technical Support command on the Visual C++
Help menu, or open the Technical Support help file for more information
INTERNAL COMPILER ERROR in 'C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\CL.exe'
Please choose the Technical Support command on the Visual C++
Help menu, or open the Technical Support help file for more information

[dzenanz]

Well, I guess you should report this instance of ICE-ing ... aaand give up on this rewrite, at least for the time being 😄

[blowekamp]

@N-Dekker Thanks for sharing and informing me about that usage of initializer lists and parameter packing.

[N-Dekker]

@dzenanz

Well, I guess you should report this instance of ICE-ing ... aaand give up on this rewrite, at least for the time being

Just reported the compiler bug to Microsoft, please "up-vote":

Visual C++ "fatal error C1001: Internal compiler error" unpacking variadic template arguments to typeid(ClassTemplate).name()...

Minimal example to reproduce the compiler bug, at https://godbolt.org/z/PnGW5hcnj :

#include <typeinfo>

template <int> class ClassTemplate {};

template <int... N>
void
VariadicFuncTemplate()
{
  const char* names[] = { typeid(ClassTemplate<N>).name()... };
}

int main()
{
  VariadicFuncTemplate<1, 2, 3>();
}

---

Update from the Microsoft Feedback Bot, January 18, 2022:

A fix for this issue has been internally implemented and is being prepared for release. We’ll update you once it becomes available for download.