Skip to content

ENH: Add FFTDiscreteGaussianImageFilter#3121

Merged
tbirdso merged 5 commits intoInsightSoftwareConsortium:masterfrom
tbirdso:gaussian-fft-filter
Jan 28, 2022
Merged

ENH: Add FFTDiscreteGaussianImageFilter#3121
tbirdso merged 5 commits intoInsightSoftwareConsortium:masterfrom
tbirdso:gaussian-fft-filter

Conversation

@tbirdso
Copy link
Copy Markdown
Contributor

@tbirdso tbirdso commented Jan 17, 2022

Changes:

  • Implements FFTDiscreteGaussianImageFilter as a subclass of DiscreteGaussianImageFilter to perform convolution of an ND single-channel input image with a Gaussian kernel of fixed width in the Fourier domain
  • Implements FFTDiscreteGaussianImageFilterEnums to select among kernel generation methods, where COMBINED_OPERATORS composes through vector multiplication the exact same ND kernel as used in DiscreteGaussianImageFilter in image format, while IMAGE_SOURCE relies on the (potentially faster) method of directly generating the kernel image through itk::GaussianImageSource
  • Exposes protected methods for generating the kernel from error/width/spacing inputs in the base DiscreteGaussianImageFilter class so that FFTDiscreteGaussianImageFilter can use the same process for generating a kernel to specifications
  • Adds FFTDiscreteGaussianImageFilterFactory for manually overriding DiscreteGaussianImageFilter for FFT acceleration benefits
  • Updates test coverage, including a test comparing FFTDiscreteGaussianImageFilter output to DiscreteGaussianImageFilter output with the same parameters
  • Includes Python wrapping

Input:
2th_cthead1

Test output screenshot:

fft-output

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.

@github-actions github-actions bot added area:Filtering Issues affecting the Filtering module type:Data Changes to testing data 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 Jan 17, 2022
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.

Looking great!

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch from 3e3ac68 to 4498057 Compare January 18, 2022 18:27
@github-actions github-actions bot added the area:Python wrapping Python bindings for a class label Jan 18, 2022
Copy link
Copy Markdown
Contributor Author

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

@thewtex @dzenanz A few design questions for you before formally moving to the review stage.

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch 2 times, most recently from 10e6c4a to 1f27fe4 Compare January 19, 2022 14:15
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 19, 2022

Pasting review notes from @jhlegarreta here (great points):

i) there is an exception thrown by the class under some condition; please exercise it;
ii) the Set methods overriding the base class should be exercised, hopefully creating the necessary cases in the CMakeLists.txt so that any if/else condition in the implementation gets exercised

Will finish addressing these and then mark as ready

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch 2 times, most recently from fa953f0 to 2e55b3d Compare January 21, 2022 19:30
@tbirdso tbirdso changed the title WIP: ENH: Add FFTDiscreteGaussianImageFilter ENH: Add FFTDiscreteGaussianImageFilter Jan 21, 2022
@tbirdso tbirdso marked this pull request as ready for review January 21, 2022 19:35
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 21, 2022

@thewtex @dzenanz Ping for re-review

Note that I've consolidated and exposed functionality from itk::DiscreteGaussianImageFilter for generating a kernel radius to error specifications with itk::GaussianOperator to avoid duplication in itk::FFTDiscreteGaussianImageFilter.

UseImageSpacing and MaximumError are now used for kernel generation.

@github-actions github-actions bot added the type:Enhancement Improvement of existing methods or implementation label Jan 21, 2022
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 21, 2022

For looking forward, I've gone ahead and added a factory override mechanism so that developers can opt in to having classes like itk::MultiResolutionPyramidImageFilter rely on FFTDiscreteGaussianImageFilter rather than DiscreteGaussianImageFilter for blurring. We can combine this with GPU-accelerated FFT overrides to speed up blurring with very large images and/or very large kernels.

If more discussion is required around the factory mechanism I'm fine with splitting it off in a separate PR.

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch 2 times, most recently from 7587368 to d3454b0 Compare January 21, 2022 22:12
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 awesome! 🎇

Tests should also be added to verify that we get the same result with the DiscreteGaussianImageFilter and the FFTDiscreteGaussianImageFilter.

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

Mostly looks good, some minor suggestions.

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch from d3454b0 to cb9abc0 Compare January 24, 2022 14:39
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 24, 2022

@thewtex 's suggestion

Tests should also be added to verify that we get the same result with the DiscreteGaussianImageFilter and the FFTDiscreteGaussianImageFilter.

was well-founded, because on more testing I am getting different images for DiscreteGaussianImageFilter and FFTDiscreteGaussianImageFilter.

DiscreteGaussianImageFilterComparisonTest:
discrete-blur-output

FFTDiscreteGaussianImageFilterTest:
fft-discrete-blur-output

Diff:
diff-output

It looks like the main issue here is that each filter relies on a different kernel source with what appears to be fundamentally different mechanisms for Gaussian kernel generation, leading to different kernel values from the same parameters.

DiscreteGaussianImageFilter creates and applies iterative 1D itk::GaussianOperators which rely on Bessel functions for defining kernel values. FFTDiscreteGaussianImageFilter requires an ND kernel image that it attempts to generate through itk::GaussianImageSource based on the kernel radii from itk::GaussianOperators constructed to the user error specifications, however the image source relies on itk::GaussianSpatialFunction to generate kernel elements which seems to produce very different values from itk::GaussianOperator.

@thewtex @dzenanz could you take a look and weigh in on this? Two potential options that I see for moving forward:

  • Dig deeper into the differences in the itk::GaussianOperator and itk::GaussianSpatialFunction procedures to understand how parameters may need to be mapped between the two for similar outputs;
  • Successively generate 1D itk::GaussianOperators, copy to itk::VariableSizeMatrix objects, cross-multiply to produce an ND kernel, iteratively copy elements to a kernel image, and update image metadata to match the input image. This would increase prep time as we may need to run 3- or 4-D matrix multiplication to get "large" kernels. However I would favor this approach as it retains a dependency on itk::GaussianOperator only and time for kernel generation was previously anticipated to be small compared to convolution.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Jan 24, 2022

Successively generate 1D itk::GaussianOperators, copy to itk::VariableSizeMatrix objects, cross-multiply to produce an ND kernel, iteratively copy elements to a kernel image, and update image metadata to match the input image. This would increase prep time as we may need to run 3- or 4-D matrix multiplication to get "large" kernels. However I would favor this approach as it retains a dependency on itk::GaussianOperator only and time for kernel generation was previously anticipated to be small compared to convolution.

👍

If it is not too much trouble and there is a performance benefit, we could keep GaussianImageSource as an option. But GaussianOperator makes sense if we are proposing a factory-enabled replacement for DiscreteGaussianImageFilter.

@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 24, 2022

Thanks @thewtex . Do you know if we multiply operators to get a kernel anywhere else in ITK? I'm having trouble finding examples of how to do this efficiently.

EDIT: It looks like VariableSizeMatrix implements 2D arrays only while we are seeking a method for 3-, 4-, or N-D matrix multiplication to properly construct the kernel. Can manually implement if necessary but it would be nice to rely on a standardized method.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Jan 24, 2022

@tbirdso nothing comes to mind offhand, but I agree -- taking a look to re-use any existing functionality makes sense, but we can also use a manual implementation.

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch from cb9abc0 to a5f0d3c Compare January 24, 2022 22:38
@tbirdso
Copy link
Copy Markdown
Contributor Author

tbirdso commented Jan 24, 2022

@dzenanz @thewtex Ping for re-review.

New changes:

  • Can use FFTDiscreteGaussianImageFilterEnums to specify how FFTDiscreteGaussianImageFilter should generate its kernel image for FFT convolution
  • If the kernel source is set to FFTDiscreteGaussianImageFilterEnums::KernelSource::COMBINED_OPERATORS then the filter will generate an empty image and manually perform vector multiplication to compose an ND Gaussian kernel image from N 1D directional kernels
  • Tests are added to exercise different kernel methods and to compare output to DiscreteGaussianImageFilter when the kernel source is COMBINED_OPERATORS

@tbirdso tbirdso force-pushed the gaussian-fft-filter branch 2 times, most recently from 81d41bd to 41bb68b Compare January 25, 2022 14:20
@tbirdso tbirdso force-pushed the gaussian-fft-filter branch from 4a0b5cb to 2786d9a Compare January 27, 2022 14:34
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.

Beautiful!

One minor issue noted inline.

`itk::DiscreteGaussianImageFilter` exposes parameters for generating a
Gaussian kernel to error and size specifications, but previously did
not make information on the kernel that was actually generated available
to other classes.

These changes expose protected methods to allow subclasses to create
kernels to the same specifications and also publicly exposes information
on the kernel sigma when image spacing is being accounted for.
Implements `FFTDiscreteGaussianImageFilter` as a subclass of
`DiscreteGaussianImageFilter` performing convolution of an ND
single-channel input image with a Gaussian kernel of fixed width in the
Fourier domain. Includes wrappings, test and baseline image.
Adds a factory class for manual override of
`DiscreteGaussianImageFilter` with `FFTDiscreteGaussianImageFilter`
through object factory instantiation.

This provides an optional method for speeding up image blurring over
very large images or using a very large kernel via GPU-accelerated FFTs
inside other classes relying on `DiscreteGaussianImageFilter` for
internal blurring (see `itkMultiResolutionPyramidImageFilter.h`).
@tbirdso tbirdso force-pushed the gaussian-fft-filter branch from 2786d9a to d1e7c9b Compare January 28, 2022 14:39
Copy link
Copy Markdown
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.

Looks good, just a few nitpicks.

@dzenanz dzenanz requested a review from thewtex January 28, 2022 15:27
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.

LGTM!

@tbirdso tbirdso merged commit 6925c8c into InsightSoftwareConsortium:master Jan 28, 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 area:Python wrapping Python bindings for a class type:Data Changes to testing data 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