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.

Sign up for GitHub

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

STYLE: Use std::abs, instead of doing if (x < 0) x = -x manually #4943

Merged
merged 1 commit into from
Nov 11, 2024
Merged

STYLE: Use std::abs, instead of doing if (x < 0) x = -x manually #4943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions Modules/Core/Common/include/itkMath.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,7 @@ FloatAlmostEqual(T x1,
return false;
}

typename Detail::FloatIEEE<T>::IntType ulps = FloatDifferenceULP(x1, x2);
Copy link
Member

@thewtex thewtex Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@N-Dekker Are you sure that this has the same behavior, in particular with -0 and 0?

Copy link
Contributor Author

@N-Dekker N-Dekker Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for bringing 0 versus -0 to the attention, Matt. In this particular case, FloatDifferenceULP returns an integer type. For integer types 0 and -0 are bit-wise equal, in C++. So exactly the same behavior, in this particular case.

if (ulps < 0)
{
ulps = -ulps;
}
typename Detail::FloatIEEE<T>::IntType ulps = std::abs(FloatDifferenceULP(x1, x2));
return ulps <= maxUlps;
}

Expand Down
8 changes: 2 additions & 6 deletions Modules/Core/Common/include/itkTriangleCell.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "vnl/algo/vnl_determinant.h"

#include <algorithm> // For copy_n.
#include <cmath> // For abs.

namespace itk
{
Expand Down Expand Up @@ -237,12 +238,7 @@ TriangleCell<TCellInterface>::DistanceToLine(PointType x,
denom += static_cast<double>(v21[i] * v21[i]);
}

// trying to avoid an expensive fabs
double tolerance = 1.e-05 * num;
if (tolerance < 0.0)
{
tolerance = -tolerance;
}
double tolerance = std::abs(1.e-05 * num);
Comment on lines -240 to +241
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment "trying to avoid an expensive fabs" appears to be introduced by commit cc69c19 "ENH: Added EvaluatePosition()...", Julien Jomier (@jjomier), Nov 16, 2004. fabs might have been expensive twenty years ago. But nowadays, it appears to be the other way around! When compiler optimization is enabled, fabs appears to be compiled to just a single machine instruction! While the manual approach (if (x < 0)...) appears to yield quite a few more machine instructions! Example at https://godbolt.org/z/M8e6x8ssh

double Call_fabs(double x)
{
    return fabs(x);
}

double Call_std_abs(double x)
{
    return std::abs(x);
}

double ManuallyEstimateAbs(double x)
{
    if (x < 0)
    {
        return -x;
    }
    return x;
}

Output:

Call_fabs(double):
        andpd   xmm0, XMMWORD PTR .LC0[rip]
        ret
Call_std_abs(double):
        andpd   xmm0, XMMWORD PTR .LC0[rip]
        ret
ManuallyEstimateAbs(double):
        pxor    xmm2, xmm2
        movapd  xmm3, xmm0
        movapd  xmm1, xmm0
        xorpd   xmm1, XMMWORD PTR .LC2[rip]
        cmpltsd xmm0, xmm2
        andpd   xmm1, xmm0
        andnpd  xmm0, xmm3
        orpd    xmm0, xmm1
        ret
.LC0:
        .long   -1
        .long   2147483647
        .long   0
        .long   0
.LC2:
        .long   0
        .long   -2147483648
        .long   0
        .long   0

if ((-tolerance < denom) && (denom < tolerance)) // numerically bad!
{
closestPoint = p1; // arbitrary, point is (numerically) far away
Expand Down
18 changes: 6 additions & 12 deletions Modules/Core/TestKernel/include/itkTestingComparisonImageFilter.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include "itkNeighborhoodAlgorithm.h"
#include "itkProgressReporter.h"

#include <cmath> // For abs.

namespace itk
{
namespace Testing
Expand Down Expand Up @@ -165,12 +167,8 @@ ComparisonImageFilter<TInputImage, TOutputImage>::ThreadedGenerateData(const Out
InputPixelType t = valid.Get();

// Assume a good match - so test center pixel first, for speed
RealType difference = static_cast<RealType>(t) - test.GetCenterPixel();
if (NumericTraits<RealType>::IsNegative(difference))
{
difference = -difference;
}
auto minimumDifference = static_cast<OutputPixelType>(difference);
RealType difference = std::abs(static_cast<RealType>(t) - test.GetCenterPixel());
auto minimumDifference = static_cast<OutputPixelType>(difference);
Comment on lines -168 to +171
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thewtex Regarding our discussion at discourse So yes, ComparisonImageFilter does take the absolute value! (This pull request just aims to make that clearer. It does not change the behavior, as far as I can see.)

Copy link
Member

Choose a reason for hiding this comment

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

@N-Dekker great!


// If center pixel isn't good enough, then test the neighborhood
if (minimumDifference > m_DifferenceThreshold)
Expand All @@ -182,12 +180,8 @@ ComparisonImageFilter<TInputImage, TOutputImage>::ThreadedGenerateData(const Out
{
// Use the RealType for the difference to make sure we get the
// sign.
RealType differenceReal = static_cast<RealType>(t) - test.GetPixel(i);
if (NumericTraits<RealType>::IsNegative(differenceReal))
{
differenceReal = -differenceReal;
}
auto d = static_cast<OutputPixelType>(differenceReal);
RealType differenceReal = std::abs(static_cast<RealType>(t) - test.GetPixel(i));
auto d = static_cast<OutputPixelType>(differenceReal);
if (d < minimumDifference)
{
minimumDifference = d;
Expand Down
8 changes: 3 additions & 5 deletions Modules/Filtering/ImageFeature/include/itkBilateralImageFilter.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include "itkTotalProgressReporter.h"
#include "itkStatisticsImageFilter.h"

#include <cmath> // For abs.

namespace itk
{
template <typename TInputImage, typename TOutputImage>
Expand Down Expand Up @@ -280,12 +282,8 @@ BilateralImageFilter<TInputImage, TOutputImage>::DynamicThreadedGenerateData(
{
// range distance between neighborhood pixel and neighborhood center
pixel = static_cast<OutputPixelRealType>(b_iter.GetPixel(i));
rangeDistance = pixel - centerPixel;
// flip sign if needed
if (rangeDistance < 0.0)
{
rangeDistance *= -1.0;
}
rangeDistance = std::abs(pixel - centerPixel);

// if the range distance is close enough, then use the pixel
if (rangeDistance < rangeDistanceThreshold)
Expand Down
9 changes: 2 additions & 7 deletions Modules/Numerics/FEM/src/itkFEMElement2DC0LinearLine.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*=========================================================================*/

#include "itkFEMElement2DC0LinearLine.h"
#include <cmath> // For abs.

namespace itk
{
Expand Down Expand Up @@ -144,13 +145,7 @@ Element2DC0LinearLine::DistanceToLine(const VectorType & x,
//
num = p21[0] * (x[0] - p1[0]) + p21[1] * (x[1] - p1[1]) + p21[2] * (x[2] - p1[2]);
denom = p21[0] * p21[0] + p21[1] * p21[1] + p21[2] * p21[2];

// trying to avoid an expensive fabs
tolerance = 1e-5 * num;
if (tolerance < 0.0)
{
tolerance = -tolerance;
}
tolerance = std::abs(1e-5 * num);
if (-tolerance < denom && denom < tolerance) // numerically bad!
{
closest = p1; // arbitrary, point is (numerically) far away
Expand Down
6 changes: 2 additions & 4 deletions Modules/Registration/Common/include/itkEuclideanDistancePointMetric.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define itkEuclideanDistancePointMetric_hxx

#include "itkImageRegionConstIteratorWithIndex.h"
#include <cmath> // For abs.

namespace itk
{
Expand Down Expand Up @@ -89,10 +90,7 @@ EuclideanDistancePointMetric<TFixedPointSet, TMovingPointSet, TDistanceMap>::Get
minimumDistance = m_DistanceMap->GetPixel(index);
// In case the provided distance map was signed,
// we correct here the distance to take its absolute value.
if (minimumDistance < 0.0)
{
minimumDistance = -minimumDistance;
}
minimumDistance = std::abs(minimumDistance);
closestPoint = true;
}
}
Expand Down