Skip to content

ENH: Add default FFT object factories to TestKernel initialization#3002

Merged
thewtex merged 3 commits intoInsightSoftwareConsortium:masterfrom
tbirdso:test-kernel-register-fft-factories
Jan 7, 2022
Merged

ENH: Add default FFT object factories to TestKernel initialization#3002
thewtex merged 3 commits intoInsightSoftwareConsortium:masterfrom
tbirdso:test-kernel-register-fft-factories

Conversation

@tbirdso
Copy link
Copy Markdown
Contributor

@tbirdso tbirdso commented Dec 27, 2021

Addresses remote module test failures in #2950 by adding FFT factories to the list of default factories to be registered with ITKTestKernel.

itkTestDriverIncludeRequiredIOFactories.h and similar strings are renamed to itkTestDriverIncludeRequiredFactories.h to reflect that FFT factories and any other future backends managed with the object factory should be default registered here. Manual FFT factory registrations previously required for testing are removed.

ITKMontage tests pass when these changes are applied.

PR Checklist

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

@github-actions github-actions Bot added area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module 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 labels Dec 27, 2021
Comment thread CMake/ITKModuleTest.cmake Outdated
Comment thread Modules/Core/TestKernel/src/itkTestDriverIncludeRequiredFactories.cxx Outdated
Comment thread Modules/Segmentation/SuperPixel/test/itkSLICImageFilterGTest.cxx
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Dec 27, 2021

It looks like https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Filtering/FFT/include/itkFFTImageFilterFactory.h should #include "itkImage.h".

@tbirdso tbirdso force-pushed the test-kernel-register-fft-factories branch from 8a7ed02 to 1595150 Compare December 27, 2021 23:22
Comment thread Modules/Filtering/FFT/include/itkFFTImageFilterFactory.h
@tbirdso tbirdso force-pushed the test-kernel-register-fft-factories branch from 46e337c to 1595150 Compare December 28, 2021 15:52
@tbirdso tbirdso force-pushed the test-kernel-register-fft-factories branch from 1595150 to b079645 Compare December 28, 2021 16:45
@tbirdso tbirdso marked this pull request as ready for review December 28, 2021 16:47
Copy link
Copy Markdown
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.

@tbirdso looking good!

To improve the consistency of the factories registered by default by the test driver, just the VNL factories can be registered. If other factories are required by a test, e.g. FFTW, CUDA, VkFFT, they should be explicitly registered or used directly be the test.

Comment thread Modules/Core/TestKernel/include/itkTestDriverIncludeBuiltInFactories.h Outdated
Comment thread Modules/Core/TestKernel/include/itkTestDriverIncludeBuiltInFactories.h Outdated
Comment thread Modules/Core/TestKernel/src/itkTestDriverIncludeRequiredFactories.cxx Outdated
Comment thread Modules/Core/TestKernel/src/itkTestDriverIncludeRequiredFactories.cxx Outdated
@thewtex
Copy link
Copy Markdown
Member

thewtex commented Dec 29, 2021

To improve the consistency of the factories registered by default by the test driver, just the VNL factories can be registered. If other factories are required by a test, e.g. FFTW, CUDA, VkFFT, they should be explicitly registered or used directly be the test.

@tbirdso @jhlegarreta this will help with #3008 , for example

@tbirdso tbirdso force-pushed the test-kernel-register-fft-factories branch from eb922f7 to 272504f Compare December 29, 2021 14:28
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Dec 29, 2021

Rebased on master.

@tbirdso tbirdso force-pushed the test-kernel-register-fft-factories branch from 073aba4 to e31ef5e Compare January 4, 2022 21:48
@tbirdso tbirdso force-pushed the test-kernel-register-fft-factories branch from e31ef5e to 0e64245 Compare January 6, 2022 14:44
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 6, 2022

@thewtex Could this please be reviewed again?

Copy link
Copy Markdown
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.

Looks great!

@tbirdso thanks for your patience and persistence.

@thewtex thewtex merged commit 9f2e6a8 into InsightSoftwareConsortium:master Jan 7, 2022
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
…ernel-register-fft-factories

ENH: Add default FFT object factories to TestKernel initialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module 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.

3 participants