ENH: Decouple itk::Math from vnl_math (constants and functions)#6427
Merged
hjmjohnson merged 14 commits intoJun 11, 2026
Merged
Conversation
github-actions
Bot
added
type:Enhancement
Member
Author
|
@greptileai review this draft before I make it official |
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
from
c032227 to
34583f4
Compare
Member
Author
|
Thanks — the P1 MSVC guard issue is real and fixed in 34583f4 (folded into the ENH commit). The Re @greptileai review |
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Jun 10, 2026
From greptile review on PR InsightSoftwareConsortium#6427 (InsightSoftwareConsortium#6427 (comment)): sqrteps is 1 ULP above sqrt(eps). The exact value of sqrt(2^-52) = 2^-26 in decimal is 1.490116119384765625e-8; the literal 1.490116119384766e-08 is 3.75e-24 above that exact value, which exceeds the half-ULP threshold (~1.65e-24) and therefore rounds to 2^-26 + 2^-78 instead of 2^-26. Since 2^-26 is a power-of-two and exactly representable, the hex float 0x1p-26 is the simplest, unambiguous fix -- and consistent with the PR's stated goal of correctly-rounded constants. Verified locally that std::sqrt(std::numeric_limits<double>::epsilon()) returns exactly 0x1p-26 (IEEE-754 sqrt is correctly rounded), so the new value equals what C++26 constexpr std::sqrt(eps) would produce.
github-actions
Bot
added
the
area:Documentation
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
3 times, most recently
from
b90b91d to
96fbc67
Compare
N-Dekker
previously requested changes
Jun 10, 2026
N-Dekker
left a comment
N-Dekker
left a comment
Contributor
There was a problem hiding this comment.
Before merging, I think possible ODR violations should be considered.
N-Dekker
reviewed
Jun 10, 2026
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Jun 10, 2026
From greptile review on PR InsightSoftwareConsortium#6427 (InsightSoftwareConsortium#6427 (comment)): sqrteps is 1 ULP above sqrt(eps). The exact value of sqrt(2^-52) = 2^-26 in decimal is 1.490116119384765625e-8; the literal 1.490116119384766e-08 is 3.75e-24 above that exact value, which exceeds the half-ULP threshold (~1.65e-24) and therefore rounds to 2^-26 + 2^-78 instead of 2^-26. Since 2^-26 is a power-of-two and exactly representable, the hex float 0x1p-26 is the simplest, unambiguous fix -- and consistent with the PR's stated goal of correctly-rounded constants. Verified locally that std::sqrt(std::numeric_limits<double>::epsilon()) returns exactly 0x1p-26 (IEEE-754 sqrt is correctly rounded), so the new value equals what C++26 constexpr std::sqrt(eps) would produce.
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
from
96fbc67 to
2905196
Compare
dzenanz
approved these changes
Jun 10, 2026
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Jun 10, 2026
From greptile review on PR InsightSoftwareConsortium#6427 (InsightSoftwareConsortium#6427 (comment)): sqrteps is 1 ULP above sqrt(eps). The exact value of sqrt(2^-52) = 2^-26 in decimal is 1.490116119384765625e-8; the literal 1.490116119384766e-08 is 3.75e-24 above that exact value, which exceeds the half-ULP threshold (~1.65e-24) and therefore rounds to 2^-26 + 2^-78 instead of 2^-26. Since 2^-26 is a power-of-two and exactly representable, the hex float 0x1p-26 is the simplest, unambiguous fix -- and consistent with the PR's stated goal of correctly-rounded constants. Verified locally that std::sqrt(std::numeric_limits<double>::epsilon()) returns exactly 0x1p-26 (IEEE-754 sqrt is correctly rounded), so the new value equals what C++26 constexpr std::sqrt(eps) would produce.
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
from
2905196 to
e99b0c2
Compare
Replace the vnl_math:: aliases with self-contained definitions: the correctly-rounded double literals (OEIS-referenced in vnl_math.h), identical under every C++ standard level. Under C++20, a static_assert block proves each literal bit-identical to its std::numbers equivalent. Defining from literals unconditionally keeps every translation unit's definition token-identical regardless of -std level, so mixing C++17 and C++20 TUs in one program cannot yield differing itk::Math values (ODR concern raised in review; see also issue InsightSoftwareConsortium#5823). Notably, std::numbers::inv_sqrtpi / std::numbers::sqrt2 double-rounds 1 ULP away from the correctly-rounded one_over_sqrt2pi literal, so definitions dispatched on __cpp_lib_math_constants would have produced standard- dependent values for that constant. This decouples itk::Math from vnl_math, whose constants are slated for deprecation and eventual removal in upstream vxl.
No VXL_VERSION macro is referenced in this header; the include and its explanatory comment described an intent the code never enforced.
From greptile review on PR InsightSoftwareConsortium#6427 (InsightSoftwareConsortium#6427 (comment)): sqrteps is 1 ULP above sqrt(eps). The exact value of sqrt(2^-52) = 2^-26 in decimal is 1.490116119384765625e-8; the literal 1.490116119384766e-08 is 3.75e-24 above that exact value, which exceeds the half-ULP threshold (~1.65e-24) and therefore rounds to 2^-26 + 2^-78 instead of 2^-26. Since 2^-26 is a power-of-two and exactly representable, the hex float 0x1p-26 is the simplest, unambiguous fix -- and consistent with the PR's stated goal of correctly-rounded constants. Verified locally that std::sqrt(std::numeric_limits<double>::epsilon()) returns exactly 0x1p-26 (IEEE-754 sqrt is correctly rounded), so the new value equals what C++26 constexpr std::sqrt(eps) would produce.
Downstream survey: ANTs uses itk::Math::sqrteps once, as an rcond() tolerance; MITK uses vnl_math::sqrteps directly for plane-distance and level-window tolerances. Neither is meaningfully affected by the 1 ULP decrease to the exactly-representable 2^-26.
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
from
e99b0c2 to
589d680
Compare
Member
Author
|
@N-Dekker the ODR concern is addressed in 62ba138: constant definitions are now standard-invariant literals (no |
hjmjohnson
changed the title
dzenanz
approved these changes
Jun 10, 2026
github-actions
Bot
added
type:Infrastructure
hjmjohnson
changed the title
…tations Replace the 14 using vnl_math::<function> declarations with native definitions preserving the historical interface: constexpr sgn, sgn0, sqr, cube, squared_magnitude, remainder_truncated, remainder_floored; rnd, rnd_halfinttoeven, rnd_halfintup, floor, ceil with the vnl std::lrint / std::floor / std::ceil bodies (full int input range); and angle_0_to_2pi, angle_minuspi_to_pi. This completes the itk::Math interface decoupling started with the constants. <vnl/vnl_math.h> remains included so transitive users keep compiling; its removal is deferred.
…lter vnl_huge_val is slated for deprecation in upstream vxl. The int overload returned INT_MAX as the initial min-energy sentinel; the type-correct Float maximum serves the same purpose.
itk::Math::abs is a backward-compatibility forwarder to Absolute, removed under ITK_FUTURE_LEGACY_REMOVE. Calling Absolute directly is future-proof and unambiguous; behavior is identical.
…MOVE itk::Math no longer uses anything from vnl_math.h. The include remains for downstream code reaching vnl_math:: transitively through itkMath.h, and disappears under ITK_FUTURE_LEGACY_REMOVE builds.
Handle the namespace-style vnl_math:: spellings (constants, functions, predicates, std re-exports, abs) alongside the legacy vnl_math_* globals, targeting C++17-compatible itk::Math/std equivalents. Report vnl_huge_val occurrences for manual, type-aware migration.
Improvements over the vnl_math variants these functions replace: - squared_magnitude(int/long/long long): multiply in the unsigned type; vnl's x * x is signed-overflow UB for |x| > sqrt(max). - remainder_floored (signed integral): single-modulo conditional add; vnl's ((x % y) + y) % y overflows the intermediate for |y| > max/2. - angle_0_to_2pi: drop an unreachable tail return inherited from vnl.
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
from
9123d26 to
f2c2a91
Compare
Parity coverage mirrors the breadth of vxl's vnl_math test suite: halfway-case rounding, floor/ceil, sign functions, powers, squared_magnitude across all overload types, the remainder sign matrix, angle normalization including the fmod boundary guard, and the constants (sqrteps's intentional 1 ULP divergence asserted explicitly).
hjmjohnson
force-pushed
the
itk-math-vnl-decouple
branch
from
f2c2a91 to
faea421
Compare
The test's purpose is calling vnl_math:: for comparison; once the VNL deprecation campaign lands, those calls would trip warnings-as-fatal CI gates. The opt-out macros are no-ops under the current VNL pin.
The VNL deprecation-campaign pin fixes vnl_math::sqrteps to the exact 2^-26; the prior pin rounds 1 ULP above. Accept either so the test passes on both sides of the VNL update.
hjmjohnson
dismissed
N-Dekker’s stale review
Already fixed! Thanks for you commment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fully decouples
itk::Mathfromvnl_math, preempting the deprecation ofvnl_math::constants and functions in upstream vxl: constants become correctly-rounded literals (C++20static_assert-verified againststd::numbers), the 14 function aliases become native implementations, and ITK-proper's last deprecated-entity uses (vnl_huge_val, legacyitk::Math::absspellings) are migrated.What changed
itkMath.hconstants: 24 OEIS-referenced double literals, token-identical in every translation unit regardless of-stdlevel; a C++20-guardedstatic_assertblock proves bit-identity tostd::numbers(18 identities;sqrt2pihas nostd::numbersconstant andone_over_sqrt2pino exact identity —inv_sqrtpi / sqrt2double-rounds 1 ULP high).itkMath.hfunctions: the 14using vnl_math::<fn>aliases (sgn,sgn0,sqr,cube,squared_magnitude,remainder_truncated/floored,rnd*,floor,ceil,angle_*) replaced by nativeconstexpr/inline implementations preserving the historical interface; rounding functions delegate to existingitk::Math::Round*/Floor/Ceil.#include <vnl/vnl_math.h>initkMath.hgated behind!ITK_FUTURE_LEGACY_REMOVE(kept transitionally for downstreamvnl_math::users).sqrtepscorrected to exactly-representable0x1p-26(vnl's decimal literal rounds 1 ULP abovesqrt(eps)); documented in the ITKv6 migration guide.vnl_math::piuses →itk::Math::pi;vnl_huge_val(0)in FEMRegistrationFilter →NumericTraits<Float>::max(); 63itk::Math::abscall sites (25 files) →itk::Math::Absolute; vestigialvnl_math.hincludes removed.VNL_ModernizeNaming.pyrewritten to migrate all deprecatedvnl_mathspellings (namespace + legacy globals) to C++17-compatibleitk::Math/std::targets;vnl_huge_valflagged for manual type-aware fixes.ODR rationale (review feedback)
Definitions dispatched on
__cpp_lib_math_constantswould give a program mixing C++17- and C++20-compiled translation units two definitions of the sameinlinevariable — and forone_over_sqrt2pithe two paths actually differed by 1 ULP. Defining every constant from its literal makes the definitions standard-invariant;std::numbersparticipates only in C++20-guardedstatic_asserts, which are not entities and cannot violate ODR. See #5823 for the general concern.Verification
itkMathVnlParityGTest.cxx(9 suites, all passing) asserts bit-for-bit equivalence of every native against itsvnl_math::counterpart: halfway-case rounding, floor/ceil, sign functions, powers, allsquared_magnitudeoverloads, the remainder sign matrix, angle normalization including the fmod boundary guard, and the constants (sqrteps's intentional 1 ULP divergence asserted explicitly).std::lrint/std::floor/std::ceil; fixed to the vnl bodies (full int range). Three vnl-inherited defects are fixed in a separate commit (ENH: Harden itk::Math natives beyond vnl_math parity): signed-overflow UB insquared_magnitude, intermediate-overflow UB in integralremainder_floored, and an unreachable tail inangle_0_to_2pi.ninjabuild: exit 0; 63/63 ctests across touched modules (Math, NIFTI, Mesh, Statistics, MathematicalMorphology).itkMath.hcompiled-fsyntax-onlyunder all four configurations:-std=c++17/c++20× default/ITK_FUTURE_LEGACY_REMOVE.std::numbersidentities: 18 EXACT;one_over_sqrt2piexpression mismatch (hence its exclusion).