InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
STYLE: Prefer `std::fabs` in float-only test contexts and add a tolerance-parameterized test macro
- 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
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