Skip to content
New issue

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

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

Already on GitHub? # to your account

STYLE: Replace std::pow(x, 2) calls with Math::sqr(x) #5249

Merged

Conversation

N-Dekker
Copy link
Contributor

Math::sqr(x) appears slightly more readable than std::pow(x, 2).

@github-actions github-actions bot added area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module type:Style Style changes: no logic impact (indentation, comments, naming) area:Numerics Issues affecting the Numerics module labels Feb 17, 2025
@issakomi
Copy link
Member

issakomi commented Feb 18, 2025

BTW, sqr sometimes means square root, e.g. in VBA.

Edit:
In e.g. Delphi sqr is square like in VNL. There is some ambiguity.

@seanm
Copy link
Contributor

seanm commented Feb 19, 2025

I had actually started writing a comment wondering why we saved 1 letter and didn't just call it sqrt like C/C++ do, then I realized it's not square root just before clicking "Comment".

Renaming it square might be wise...

@N-Dekker
Copy link
Contributor Author

Thanks @issakomi and @seanm I don't like the name sqr either. But it's taken directly from the vxl library:

using vnl_math::sqr;

itk::Math::sqr was added to ITK almost nine years ago, with commit 2133446, from 2016.

If it would be implemented in ITK instead, I would prefer it to be named Square, following ITK's "Pascal Case" naming convention.

Anyway, my aim for this PR is to avoid calling std::pow unnecessarily, as it appears somewhat troublesome, as reported by issue #5245

I feel that a new square function for ITK is beyond the scope of this PR. (The new square function might be constexpr, it might be written as a function template, it would need extra unit tests, deprecating the current sqr, considering what to do about Math::cube, etc.) Do you agree that it would be an acceptable improvement to at least replace std::pow(x, 2) calls with the old Math::sqr(x), and then possibly later introduce a new square function?

@N-Dekker N-Dekker marked this pull request as ready for review February 19, 2025 15:44
@@ -128,7 +128,7 @@ StatisticsLabelMapFilter<TImage, TFeatureImage>::ThreadedProcessLabelObject(Labe

// increase the sums
sum += v;
sum2 += std::pow(static_cast<double>(v), 2);
sum2 += Math::sqr(static_cast<double>(v));
Copy link
Contributor

@seanm seanm Feb 19, 2025

Choose a reason for hiding this comment

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

This one is arguably better left as it was, to be more similar to the next 2 lines... but I don't feel strongly about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, thanks, but on the other hand, the next line (sum3 += std::pow(static_cast<double>(v), 3)) might as well use Math::cube instead 😸

Comment on lines -105 to +106
m_Radius >= std::sqrt(std::pow(pointVector.GetNorm(), 2.0) - std::pow(distanceFromCenter, 2.0)))
m_Radius >= std::sqrt(Math::sqr(pointVector.GetNorm()) - Math::sqr(distanceFromCenter)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a follow-up, it might be nice to replace Math::sqr(pointVector.GetNorm() with pointVector.GetSquaredNorm(), but I guess doing so might introduce regression errors, because it would potentially reduce rounding errors!

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 that distanceFromCenter is in fact the dot product of two vectors:

const double distanceFromCenter = dot_product(medialAxisVector.GetVnlVector(), pointVector.GetVnlVector());

So I'm just wondering if the square of distanceFromCenter was originally intended. (But I will leave it "squared" like this, because I don't know the original intention.)

`Math::sqr(x)` appears slightly more readable than `std::pow(x, 2)`.
@N-Dekker N-Dekker force-pushed the Replace-pow-with-sqr branch from d349adf to 30a11da Compare February 21, 2025 09:33
@dzenanz
Copy link
Member

dzenanz commented Feb 22, 2025

This is a step in the right direction. If someone has time, this might be improved further.

@dzenanz dzenanz merged commit 20e2f6d into InsightSoftwareConsortium:master Feb 22, 2025
16 checks passed
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Feb 28, 2025
Following ITK pull request InsightSoftwareConsortium/ITK#5249 commit InsightSoftwareConsortium/ITK@20e2f6d "STYLE: Replace `std::pow(x, 2)` calls with `Math::sqr(x)`"
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Feb 28, 2025
Following ITK pull request InsightSoftwareConsortium/ITK#5249 commit InsightSoftwareConsortium/ITK@20e2f6d "STYLE: Replace `std::pow(x, 2)` calls with `Math::sqr(x)`"
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants