Skip to content

ENH: Deprecate vnl_determinant/vnl_det for Eigen-backed itk::Math::Determinant#6568

Open
hjmjohnson wants to merge 4 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-deprecate-vnl-determinant
Open

ENH: Deprecate vnl_determinant/vnl_det for Eigen-backed itk::Math::Determinant#6568
hjmjohnson wants to merge 4 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-deprecate-vnl-determinant

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

Deprecates vnl_determinant and vnl_det in favor of the Eigen-backed itk::Math::Determinant, migrates every in-tree consumer, and gates both vnl spellings behind the standard three-state deprecation guard. Part of the VNL→Eigen numerics migration (#6403, #6230); same shape as the vnl_matrix_exp (#6454) and vnl_fft (#6441) precedents.

Only Mac clang was exercised locally; Windows/MSVC (the #pragma message guard branch) and the Linux GCC configs rely on CI.

Commit structure (4 thematic commits)
  1. Add itk::Math::Determinant — Eigen-backed, no-copy Eigen::Map, overloads for itk::Matrix / vnl_matrix_fixed / vnl_matrix; runtime engine dispatches n≤4 to the compile-time direct formula so small runtime matrices stay fast. A forward declaration of itk::Matrix breaks the include cycle with itkMatrix.h.
  2. Test the API against vnl (10 GoogleTests: analytic 2×2/3×3, overload consistency, identity/diagonal, singular, large triangular, c^n scaling, float, non-square throw, vnl equivalence on well- and ill-conditioned inputs).
  3. Migrate consumers — all 20 call sites of vnl_determinant and vnl_det (transforms, mesh cells, image base/extract/series-writer, displacement-field Jacobian, quasi-Newton, membership/Mahalanobis, label geometry, FDF IO, video stream). itkMatrix.h/itkImageBase keep the vnl header reachable under #if !defined(ITK_LEGACY_REMOVE) && !defined(ITK_FUTURE_LEGACY_REMOVE) so downstream code relying on the transitive include still compiles during the deprecation window.
  4. Deprecate vnl_determinant.h and vnl_det.h with the three-state guard (silent default / #warning under ITK_LEGACY_REMOVE / #error under ITK_FUTURE_LEGACY_REMOVE), inert for the upstream VXL build and honoring ITK_LEGACY_TEST.
Benchmark gate — accuracy + speed

Eigen ties-or-wins both axes across the meaningful range; the vnl_* engine is therefore left in place (not auto-routed to Eigen) — consumers get Eigen via the new API, the vnl spellings keep their own engine behind the deprecation guard.

Regime Accuracy Speed
fixed 2×2 / 3×3 (itk::Matrix) tie (±1 ULP) vnl ~1 ns faster
fixed 4×4 tie Eigen 2.3×
dynamic n≤4 (incl. per-pixel Jacobian) tie / Eigen better parity–2× Eigen
dynamic n≥8 Eigen ≤ vnl Eigen 1.5–11×
ill-conditioned (Hilbert) Eigen up to 20× more accurate Eigen ~2×

Key design point: Eigen's Dynamic-sized .determinant() always uses LU, so a naive runtime path was ~30× slower than vnl for n=3/4; dispatching small n to the compile-time formula (as vnl does internally) keeps the one hot consumer — per-pixel 3×3 DisplacementFieldJacobianDeterminantFilter — at parity instead of a 30× regression.

macOS arm64, clang -O3, single-threaded, median of 200 reps, accuracy vs long-double LU reference.

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:ThirdParty Issues affecting the ThirdParty module area:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Jul 9, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review July 9, 2026 11:14
@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds an Eigen-backed determinant helper and migrates ITK away from VNL determinant calls. The main changes are:

  • New itk::Math::Determinant overloads for itk::Matrix, vnl_matrix_fixed, and vnl_matrix.
  • In-tree determinant call sites moved from vnl_determinant and vnl_det to the new Math API.
  • VNL determinant headers gated with ITK legacy deprecation warnings and future-removal errors.
  • GoogleTest coverage added for fixed, dynamic, non-square, singular, scaling, and VNL parity cases.

Confidence Score: 5/5

This PR is safe to merge with low risk.

The new determinant helper uses ITK's Eigen include macro, migrated call sites preserve the prior determinant checks, and tests cover the key overload and parity paths.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex confirmed the build environment by reviewing the determinant-related tool availability log, which shows cmake, ninja, and pixi are not found while a C++ compiler is available.
  • T-Rex examined the standalone determinant probe source to confirm it includes the new determinant API and builds a simple 2x2 itk::Matrix determinant check.
  • T-Rex reviewed the standalone determinant probe compile transcript, which shows the compile command and a fatal error due to itkConfigure.h being missing.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/Core/Common/include/itkMathDeterminant.h Adds Eigen-backed determinant overloads for itk::Matrix, fixed VNL, and dynamic VNL matrices; Eigen includes follow ITK macro policy.
Modules/Core/Common/include/itkMatrix.h Switches inverse singular checks to the new determinant helper while preserving legacy transitive VNL include during the deprecation window.
Modules/Core/Common/test/itkMathDeterminantGTest.cxx Adds determinant coverage for overload consistency, singular/non-square behavior, scaling, and VNL parity.
Modules/Filtering/DisplacementField/include/itkDisplacementFieldJacobianDeterminantFilter.hxx Migrates physical Jacobian determinant computation to itk::Math::Determinant.
Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/vnl_determinant.h Adds ITK deprecation guards for vnl_determinant while preserving upstream VXL use when itkConfigure.h is unavailable.
Modules/ThirdParty/VNL/src/vxl/core/vnl/vnl_det.h Adds matching ITK deprecation guards for vnl_det.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller as ITK call site
participant Math as itk::Math::Determinant
participant Dispatch as Fixed/dynamic dispatch
participant Eigen as Eigen determinant engine
participant Legacy as vnl_determinant/vnl_det headers

Caller->>Math: pass itk::Matrix or vnl_matrix(_fixed)
Math->>Dispatch: validate square dynamic inputs
alt "fixed size or n <= 4"
    Dispatch->>Eigen: Map row-major data to fixed-size matrix
else "dynamic n > 4"
    Dispatch->>Eigen: Map row-major data to dynamic matrix
end
Eigen-->>Caller: determinant value
Caller-->>Legacy: no migrated in-tree calls
Legacy-->>Caller: warn/error only under legacy guard macros
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller as ITK call site
participant Math as itk::Math::Determinant
participant Dispatch as Fixed/dynamic dispatch
participant Eigen as Eigen determinant engine
participant Legacy as vnl_determinant/vnl_det headers

Caller->>Math: pass itk::Matrix or vnl_matrix(_fixed)
Math->>Dispatch: validate square dynamic inputs
alt "fixed size or n <= 4"
    Dispatch->>Eigen: Map row-major data to fixed-size matrix
else "dynamic n > 4"
    Dispatch->>Eigen: Map row-major data to dynamic matrix
end
Eigen-->>Caller: determinant value
Caller-->>Legacy: no migrated in-tree calls
Legacy-->>Caller: warn/error only under legacy guard macros
Loading

Reviews (1): Last reviewed commit: "ENH: Deprecate vnl_determinant and vnl_d..." | Re-trigger Greptile

@dzenanz dzenanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't taken a look at the tests. The rest looks good on a glance.

@hjmjohnson hjmjohnson force-pushed the enh-deprecate-vnl-determinant branch from 788161a to b67f657 Compare July 9, 2026 19:53
@hjmjohnson hjmjohnson requested a review from thewtex July 10, 2026 21:13
Eigen-backed replacement for vnl_determinant/vnl_det. Overloads accept an
itk::Matrix, a vnl_matrix_fixed, or a runtime vnl_matrix over a zero-copy
Eigen::Map; no Eigen type appears in the interface. Fixed sizes and runtime
sizes <= 4x4 use direct cofactor formulas; larger runtime sizes use
PartialPivLU. The runtime engine dispatches small n to the compile-time
direct formula so small runtime matrices stay fast (Eigen's Dynamic-sized
determinant otherwise always uses LU).

A forward declaration of itk::Matrix keeps the header free of a cycle with
itkMatrix.h; the Matrix overload instantiates where itk::Matrix is complete.

Part of the VNL->Eigen numerics migration (InsightSoftwareConsortium#6403, InsightSoftwareConsortium#6230).
Scavenges the vnl test_determinant coverage into a GoogleTest exercising the
itk::Math::Determinant API: 2x2/3x3 closed-form references, overload
consistency, identity/diagonal, singular, large triangular, the c^n scaling
law, float, a non-square throw, and equivalence with vnl_det/vnl_determinant
on well- and ill-conditioned inputs. Marked ITK_LEGACY_TEST so it keeps
compiling once the vnl spellings warn under ITK_LEGACY_REMOVE.
Switches every in-tree determinant call site off vnl_determinant/vnl_det to
the Eigen-backed itk::Math::Determinant: transforms, mesh cells, image
base/extract/series writer, displacement-field Jacobian, quasi-Newton,
membership/Mahalanobis statistics, label geometry, FDF IO, and video stream.
Removes now-unused vnl determinant includes. itkMatrix.h and itkImageBase
keep the vnl header reachable under a silent-window guard so downstream code
that relied on the transitive include still compiles during the deprecation
window.
Adds the itkConfigure-gated three-state deprecation guard to both vnl
determinant headers, pointing at itk::Math::Determinant: silent by default,
a #warning under ITK_LEGACY_REMOVE, and a hard #error under
ITK_FUTURE_LEGACY_REMOVE. The guard is inert for the upstream VXL build
(fires only for an ITK consumer) and honors ITK_LEGACY_TEST.

Part of the VNL->Eigen numerics migration (InsightSoftwareConsortium#6403, InsightSoftwareConsortium#6230).
@hjmjohnson hjmjohnson force-pushed the enh-deprecate-vnl-determinant branch from b67f657 to 7ad449c Compare July 10, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:ThirdParty Issues affecting the ThirdParty module area:Video Issues affecting the Video module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants