InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

STYLE: Prefer `std::fabs` in float-only test contexts and add a tolerance-parameterized test macro

Open
#6,738 0 comments 1 reaction 0 assignees View on GitHub
type:Enhancement type:Testing
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

ITK test code frequently hand-rolls `itk::Math::Absolute(a - b) < tolerance`. In float-only contexts `std::fabs` reads more directly, and a tolerance-parameterized assertion macro would express intent and produce a better failure message than `ITK_TEST_EXPECT_TRUE` on a bare bool. Raised by @N-Dekker in review of #6108 and explicitly deferred there.

Origin — N-Dekker's review remarks on PR #6108

PR #6108 (`COMP: Replace legacy itk::Math::abs with itk::Math::Absolute in tests`) replaced the deprecated `itk::Math::abs` alias with the canonical `itk::Math::Absolute`. On `Modules/Numerics/FEM/test/itkFEMElement2DC0LinearQuadrilateralStressTestFEMObjectReader.cxx:127` the reviewer commented:

> Two remarks:
> - When the intention is to get the absolute value of a floating point number, I think `std::fabs` should be fine as well.
> - For a GoogleTest, it would be more like `EXPECT_NEAR(solver->GetSolution(dof), 0.0, tolerance)`
>
> Anyway, no problem!

and added "No need to address the two remarks I did with this PR". `Absolute` was the correct choice for #6108 (it is the post-`ITK_FUTURE_LEGACY_REMOVE` replacement for `itk::Math::abs`); this issue captures the deferred polish so it is not lost with the PR thread.

Neither remark is a bug or a regression. Both are ergonomics wins.

Current state on main (verified 2026-07-29)

- `itk::Math::Absolute` is defined at `Modules/Core/Common/include/itkMath.h:1134`, documented from `:1116`. Its doc block is explicit that `Absolute` differs from `std::abs` in return type (promotes to unsigned for integral inputs) and in supporting `bool` — i.e. `Absolute` is the right tool for *integral/generic* contexts, and is doing no useful extra work in float-only contexts.
- `Modules/Core/TestKernel/include/itkTestingMacros.h` provides **no** tolerance-parameterized assertion. The full macro list is `ITK_TEST_EXPECT_TRUE`, `ITK_TEST_EXPECT_TRUE_STATUS_VALUE`, `ITK_TEST_EXPECT_EQUAL`, `ITK_TEST_EXPECT_EQUAL_STATUS_VALUE`, `ITK_TEST_SET_GET`, `ITK_TEST_SET_GET_VALUE`, `ITK_TEST_SET_GET_NULL_VALUE`, `ITK_TEST_SET_GET_BOOLEAN`. So the second half of the sweep requires *adding* a macro, not just using one.
- Scale of the pattern:
```
$ git grep -n "Math::Absolute([^;]*-[^;]*)[[:space:]]*[<>]" upstream/main -- 'Modules/*/test/*' | wc -l
775
```
(roughly half of those lines are the `std::cerr << "Error: itk::Math::Absolute( ... )"` diagnostic echoing the condition, so the real call-site count is on the order of ~380.)
- Representative shape, `Modules/Core/Common/test/SparseLUSolverTraitsGTest.cxx:38`:
```cpp
if (itk::Math::Absolute(v1(i) - v2(i)) > tolerance)
{
std::cerr << "Error: itk::Math::Absolute( v1(" << i << ") - v2(" << i << ") ) > " << tolerance << std::endl;
...
}
```
In a GoogleTest file such as this one, `EXPECT_NEAR(v1(i), v2(i), tolerance)` is available today and subsumes both the check and the diagnostic.

Suggested approach

1. Enumerate call sites: `rg 'itk::Math::Absolute\([^)]+-[^)]+\)\s*[<>]'` (the `abs(a - b) < tol` shape).
2. **GoogleTest files** (`*GTest.cxx`): replace the hand-rolled check + `std::cerr` block with `EXPECT_NEAR(a, b, tolerance)` — strictly better failure messages, no new machinery needed. This is the highest-value subset.
3. **Non-GoogleTest test drivers**: either leave alone, or add an `ITK_TEST_EXPECT_NEAR(lh, rh, tolerance)` macro to `Modules/Core/TestKernel/include/itkTestingMacros.h` alongside `ITK_TEST_EXPECT_EQUAL` and convert. Adding the macro is a prerequisite, so this half may be scoped to a separate PR.
4. **`std::fabs` for float-only sites**: apply only where the operands are unambiguously floating point. Do **not** touch integral or complex call sites — `Absolute` is the correct choice there (see its doc block on integral promotion).
5. Prefer a single `STYLE:` commit for the mechanical sweep rather than per-file commits. Suggested subject: `STYLE: Prefer std::fabs over itk::Math::Absolute in float-only test contexts`.
6. Non-urgent, zero behavior change. Fine to pick up piecemeal during an unrelated touch of a given test file.

Related

- PR #6108 — `COMP: Replace legacy itk::Math::abs with itk::Math::Absolute in tests` (origin of the remarks)
- Reviewer: @N-Dekker
- `Modules/Core/Common/include/itkMath.h:1134` — `itk::Math::Absolute`
- `Modules/Core/TestKernel/include/itkTestingMacros.h` — where an `ITK_TEST_EXPECT_NEAR` would live

Contributor guide

Open the contributing guide

Research direction

Start with Modules/Core/Common/test/SparseLUSolverTraitsGTest.cxx and enumerate matching sites with the suggested rg command. Read itkMath.h around itk::Math::Absolute and itkTestingMacros.h; use EXPECT_NEAR in GoogleTest files, consider the separate macro work for other drivers, and limit std::fabs to clearly floating-point operands. Done means the selected sweep preserves behavior and tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.