Skip to content

Update link INRIA, improve style RecursiveGaussianImageFilter and RecursiveSeparableImageFilter#5687

Merged
thewtex merged 6 commits intoInsightSoftwareConsortium:mainfrom
N-Dekker:Make-RecursiveGaussianImageFilter-ComputeNCoefficients-static
Dec 15, 2025
Merged

Update link INRIA, improve style RecursiveGaussianImageFilter and RecursiveSeparableImageFilter#5687
thewtex merged 6 commits intoInsightSoftwareConsortium:mainfrom
N-Dekker:Make-RecursiveGaussianImageFilter-ComputeNCoefficients-static

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

The INRIA report by Rachid Deriche, "Recursively Implementing The Gaussian and Its Derivatives", 1993, appears to be moved to https://inria.hal.science/inria-00074778

This pull request also proposes various style improvements to RecursiveGaussianImageFilter and its base class RecursiveSeparableImageFilter.

Following MISRA-C++ Rule "If a member function can be made static then it shall
be made static" and Clang-Tidy `readability-convert-member-functions-to-static`,
https://clang.llvm.org/extra/clang-tidy/checks/readability/convert-member-functions-to-static.html
Following C++ Core Guidelines, Jul 8, 2025, "Don’t define a default constructor
that only initializes data members; use default member initializers instead",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c45-dont-define-a-default-constructor-that-only-initializes-data-members-use-default-member-initializers-instead
Removed the member initializer list from the default-constructor, used default
member initializers in the class definition instead.
@github-actions github-actions bot added the area:Filtering Issues affecting the Filtering module label Dec 14, 2025
Comment on lines -93 to -110
A1[0] = static_cast<ScalarRealType>(1.3530);
B1[0] = static_cast<ScalarRealType>(1.8151);
W1 = static_cast<ScalarRealType>(0.6681);
L1 = static_cast<ScalarRealType>(-1.3932);
A2[0] = static_cast<ScalarRealType>(-0.3531);
B2[0] = static_cast<ScalarRealType>(0.0902);
W2 = static_cast<ScalarRealType>(2.0787);
L2 = static_cast<ScalarRealType>(-1.3732);

A1[1] = static_cast<ScalarRealType>(-0.6724);
B1[1] = static_cast<ScalarRealType>(-3.4327);
A2[1] = static_cast<ScalarRealType>(0.6724);
B2[1] = static_cast<ScalarRealType>(0.6100);

A1[2] = static_cast<ScalarRealType>(-1.3563);
B1[2] = static_cast<ScalarRealType>(5.2318);
A2[2] = static_cast<ScalarRealType>(0.3446);
B2[2] = static_cast<ScalarRealType>(-2.2355);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the record, it appears that these "magic numbers" were originally introduced with commit ef006d2, Luis Ibanez, Jan 17, 2005. They are also presented in Table 3. "Proposed new parameters in the approximation of the Gaussian and its derivatives...", in "Improving Deriche-style Recursive Gaussian Filters", by Gunnar Farnebäck & Carl-Fredrik Westin, Nov 30 2006.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it worth documenting in the code the source of these numbers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@blowekamp Good suggestion, thanks! As follows?

  // Parameters of exponential series.
  // These numbers correspond with Table 3. "Proposed new parameters in the approximation of the Gaussian and its
  // derivatives..." of "Improving Deriche-style Recursive Gaussian Filters" [farneback2006].

I hope it's clear enough that [farneback2006] refers to our bib entry at

@article{farneback2006,
title = {Improving Deriche-style Recursive Gaussian Filters},
author = {Farneb{\"a}ck, Gunnar and Westin, Carl-Fredrik},
year = 2006,
journal = {Journal of Mathematical Imaging and Vision},
volume = 26,
number = 3,
pages = {293--299},
doi = {10.1007/s10851-006-8464-z},
url = {https://doi.org/10.1007/s10851-006-8464-z}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it is to me as it's in the class documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To be added to a follow-up 😃

Aims to clarify the code of `RecursiveSeparableImageFilter::FilterDataArray`.
Removed unnecessary `static_cast<ScalarRealType>`s.
@N-Dekker N-Dekker force-pushed the Make-RecursiveGaussianImageFilter-ComputeNCoefficients-static branch from a055224 to 5878389 Compare December 14, 2025 16:44
@N-Dekker N-Dekker marked this pull request as ready for review December 14, 2025 21:06
@blowekamp blowekamp self-requested a review December 15, 2025 12:45
Copy link
Copy Markdown
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

The style improvements look good to me.

Comment on lines -93 to -110
A1[0] = static_cast<ScalarRealType>(1.3530);
B1[0] = static_cast<ScalarRealType>(1.8151);
W1 = static_cast<ScalarRealType>(0.6681);
L1 = static_cast<ScalarRealType>(-1.3932);
A2[0] = static_cast<ScalarRealType>(-0.3531);
B2[0] = static_cast<ScalarRealType>(0.0902);
W2 = static_cast<ScalarRealType>(2.0787);
L2 = static_cast<ScalarRealType>(-1.3732);

A1[1] = static_cast<ScalarRealType>(-0.6724);
B1[1] = static_cast<ScalarRealType>(-3.4327);
A2[1] = static_cast<ScalarRealType>(0.6724);
B2[1] = static_cast<ScalarRealType>(0.6100);

A1[2] = static_cast<ScalarRealType>(-1.3563);
B1[2] = static_cast<ScalarRealType>(5.2318);
A2[2] = static_cast<ScalarRealType>(0.3446);
B2[2] = static_cast<ScalarRealType>(-2.2355);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it worth documenting in the code the source of these numbers?

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.

@N-Dekker thanks 🙏

@thewtex thewtex merged commit 589d883 into InsightSoftwareConsortium:main Dec 15, 2025
17 checks passed
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants