Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Select Forward1DFFTImageFilter backend with object factory #2836

Merged

Conversation

tbirdso
Copy link
Contributor

@tbirdso tbirdso commented Oct 25, 2021

Implement object factory methods taking advantage of existing CMake infrastructure such that instantiating an FFT filter with itk::Forward1DFFTImageFilter<imagetype>::New() automatically returns a backend from registered object factory methods. This is advantageous across several scenarios:

  • Selects vnl backend by default (itkVnlForward1DFFTImageFilter);
  • Automatically selects FFTW backend if user has indicated availability (and accepted license)
  • Opens FFT backend implementation for extension in remote modules such as ITKVkFFTBackend making use of APIs for GPU-accelerated FFT

This PR sets up the FFT factory framework based on IO factory architecture with a few small special cases for naming. Future work will involve:

  • Extending object factory functionality to all FFT classes
  • Injecting default factory registration for Python with igenerator.py
  • Improving object factory architecture for CMake targets with itk_configure_factory macro

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 Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5

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

@tbirdso tbirdso changed the title WIP: Try adding factory classes in fft class files WIP: Adding factory classes in fft class files Oct 25, 2021
@tbirdso tbirdso changed the title WIP: Adding factory classes in fft class files WIP: Adding FFT object factory methods Oct 25, 2021
@github-actions github-actions bot added area:Filtering Issues affecting the Filtering module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Oct 25, 2021
@tbirdso tbirdso force-pushed the add-1d-fft-factory branch from 3cd631c to b6b9c08 Compare October 25, 2021 14:40
Copy link
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. One comment about UnRegister.

@tbirdso
Copy link
Contributor Author

tbirdso commented Oct 25, 2021

Seeing new failures in VNL FFT classes after trying to add object factory methods (see https://open.cdash.org/viewBuildError.php?buildid=7533182):

Modules/Filtering/FFT/include/itkVnlComplexToComplex1DFFTImageFilter.hxx:88:11: error: no matching function for call to 'vnl_fft_1d::bwd_transform(VNLVectorType&)'

Modules/Filtering/FFT/include/itkVnlComplexToComplex1DFFTImageFilter.hxx:101:11: error: no matching function for call to 'vnl_fft_1d::fwd_transform(VNLVectorType&)'

Am I on the right path with registering new constructors with each FFT object factory? Not sure what's missing here.

@thewtex
Copy link
Member

thewtex commented Oct 26, 2021

@tbirdso looks to be generally on the right path. The build error sounds like a type mis-match.

@tbirdso tbirdso force-pushed the add-1d-fft-factory branch from b6b9c08 to 2f2165c Compare October 29, 2021 15:37
@github-actions github-actions bot added the type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct label Oct 29, 2021
CMake/UseITK.cmake Outdated Show resolved Hide resolved
Comment on lines 17 to 18
FACTORY_NAMES
FFT::VnlForward1D
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the syntax defined here generally mirrors FACTORY_NAMES syntax for IO classes, with minor corrections happening in UseITK.cmake. Namely, FFT::<fft-type> maps to a <fft-type>FFTImageFilterFactoryRegister__Private() function header in itkFFTImageFilterFactoryRegisterManager.h which must be implemented in a .cxx source file.

@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 1, 2021

Starting to have success in using object factories to generate a default Vnl backend for itk::Forward1DFFTImageFilter (woo!), but would appreciate extra insight into how ITK handles the CMake / export side of object factory implementation details.

@dzenanz @thewtex @N-Dekker Do you have any thoughts on the above discussions? Thanks in advance for your help.

@dzenanz
Copy link
Member

dzenanz commented Nov 1, 2021

If you want something accessible in bottom scope, you should define in in parent scope before entering that subdirectory.

@tbirdso tbirdso force-pushed the add-1d-fft-factory branch from 8996adb to 7cc15b5 Compare November 1, 2021 20:16
@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 2, 2021

Per @thewtex it is valid to require tests to manually register FFT object factories with the understanding that consuming cxx projects will call UseITK.cmake for appropriately injecting factory code and Python projects already run UseITK.cmake for setting up factories at build time by default. Hence we are much closer to a functioning change here than previously thought 😃

@tbirdso tbirdso force-pushed the add-1d-fft-factory branch from 97e8f78 to d8a5c26 Compare November 2, 2021 22:31
@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 4, 2021

Current PR status:

  • Cxx tests build and pass locally. Factories are manually registered.
    - [ ] Python tests do not pass locally. Factories are not being registered as expected?
  • FFT example modified for Forward1DFFTImageFilter builds and passes. Verified with dynamic_cast that default factories are loaded and the first constructed FFT instance has a Vnl backend.

@thewtex
Copy link
Member

thewtex commented Nov 4, 2021

 Python tests do not pass locally. Factories are not being registered as expected?

We may want to bite the bullet and inject the factory registration call for wrapped factory classes call in igenerator.py.

@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 4, 2021

 Python tests do not pass locally. Factories are not being registered as expected?

We may want to bite the bullet and inject the factor registration call for wrapped factory classes call in igenerator.py.

@thewtex I can take a look. Do you have any thoughts on how ImageIO default factories may be registered while FFT factories are not?

@thewtex
Copy link
Member

thewtex commented Nov 4, 2021

Do you have any thoughts on how ImageIO default factories may be registered while FFT factories are not?

This may be a side effect of wrapping itk.ImageFileWriter, itk.ImageFileReader.

@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 4, 2021

This may be a side effect of wrapping itk.ImageFileWriter, itk.ImageFileReader.

I was worried about that 😅 I suppose it's not reasonable for object factory classes to always have another class like ImageFileWriter, etc. to handle factory registration. I'll start digging into igenerator.py.

@github-actions github-actions bot added the area:Python wrapping Python bindings for a class label Nov 4, 2021
@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 4, 2021

Python wrapping seems to be working now, maybe due to wrapping VnlForward1DFFTImageFilter? Wrapping does not seem to be required for IO factories to work, so... 🤷‍♂️

I've confirmed on my side that the Forward1DFFTImageFilter constructor in Python returns a Vnl backend by default:

>>> import itk
>>> itk.auto_progress(2)
>>> f = itk.Forward1DFFTImageFilter[itk.Image[itk.F,2]].New()
Loading ITKPyBase... done
Loading ITKCommon... done
Loading ITKStatistics... done
Loading ITKImageFilterBase... done
Loading ITKTransform... done
Loading ITKImageFunction... done
Loading ITKImageGrid... done
Loading ITKFFT... done
Loading ITKPyUtils... done
>>> itk.VnlForward1DFFTImageFilter[itk.Image[itk.F,2]].cast(f)
<itk.itkVnlForward1DFFTImageFilterPython.itkVnlForward1DFFTImageFilterIF2; proxy of <Swig Object of type 'itkVnlForward1DFFTImageFilterIF2 *' at 0x00000209EFF4E420> >

Injecting factory calls in igenerator.py and targeting CMake projects for factory default registration with itk_configure_factory would both be directly relevant to FFT object factory registration and can be explored in subsequent PRs.

@tbirdso tbirdso force-pushed the add-1d-fft-factory branch from 2eb86b8 to eacef8b Compare November 4, 2021 19:50
@tbirdso tbirdso force-pushed the add-1d-fft-factory branch from eacef8b to 67762ee Compare November 4, 2021 20:08
@tbirdso tbirdso changed the title WIP: Adding FFT object factory methods ENH: Select Forward1DFFTImageFilter backend with object factory Nov 4, 2021
@tbirdso tbirdso marked this pull request as ready for review November 4, 2021 20:15
@github-actions github-actions bot added the type:Enhancement Improvement of existing methods or implementation label Nov 5, 2021
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

🎉 🌮

CMake/UseITK.cmake Outdated Show resolved Hide resolved
Modules/Filtering/FFT/test/CMakeLists.txt Outdated Show resolved Hide resolved
@thewtex thewtex merged commit 51c63cf into InsightSoftwareConsortium:master Nov 9, 2021
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 area:Python wrapping Python bindings for a class type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots 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.

6 participants