Skip to content

ENH: Add vnl_svd engine coverage as itkVnlSVDEngineGTest#6546

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-vnl-svd-engine-gtest
Jul 6, 2026
Merged

ENH: Add vnl_svd engine coverage as itkVnlSVDEngineGTest#6546
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-vnl-svd-engine-gtest

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

Adapt the vxl core/vnl/algo/tests/test_svd.cxx cases as an ITK GoogleTest (itkVnlSVDEngineGTest) so vnl_svd behavior is exercised directly in ITK CI. ITK relies on vnl_svd throughout registration and reconstruction, but the vxl test that covers its engine never runs in ITK's build; this makes any future change to the underlying SVD engine (or migration off vnl_svd) reviewable against an in-tree regression net.

Coverage

Six cases across float / double / complex-float / complex-double:

  • Hilbert-matrix inversion (ill-conditioned accuracy)
  • Least-squares parabola fit (solve)
  • Rank and nullspace extraction on rank-deficient inputs
  • Identity singular values
  • U·W·Vᴴ recomposition
  • nullvector() correctness
Testing performed

Built and run locally against current main (f2c LINPACK svdc engine), macOS arm64, clang: 6/6 pass. pre-commit run --all-files clean.

@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 type:Coverage Code coverage impacts labels Jul 3, 2026
Adapt the vxl core/vnl/algo/tests/test_svd.cxx cases as an ITK
GoogleTest so vnl_svd behavior is exercised in ITK CI, guarding any
future change of the underlying SVD engine. Covers Hilbert inversion,
least-squares solve, rank/nullspace extraction, identity singular
values, recomposition, and nullvector across
float/double/complex-float/complex-double.
@hjmjohnson hjmjohnson force-pushed the enh-vnl-svd-engine-gtest branch from 6fbc9c6 to f386a36 Compare July 6, 2026 12:24
@hjmjohnson hjmjohnson marked this pull request as ready for review July 6, 2026 14:38
@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds direct GoogleTest coverage for the vnl_svd engine in ITK. The main changes are:

  • New itkVnlSVDEngineGTest.cxx coverage for inversion, least-squares solve, rank/nullspace, singular values, recomposition, and nullvector behavior.
  • Test coverage across float, double, std::complex<float>, and std::complex<double> cases.
  • Registration of the new test source in the existing ITKCommon GoogleTest driver.

Confidence Score: 5/5

Safe to merge with minimal risk.

The change is test-only, follows the existing ITKCommon GoogleTest registration pattern, and does not add production or security-sensitive code. No blocking or non-blocking issues were identified.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex ran the requested verification to validate the contract, but its local artifact references were not uploaded.

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/Core/Common/test/CMakeLists.txt Registers the new itkVnlSVDEngineGTest.cxx source in the existing ITKCommon GoogleTest driver.
Modules/Core/Common/test/itkVnlSVDEngineGTest.cxx Adds deterministic GoogleTest coverage for vnl_svd inversion, solve, rank/nullspace, recomposition, singular values, and nullvector behavior across real and complex scalar types.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CMake as ITKCommon CMake
participant Driver as ITKCommonGTests
participant Test as itkVnlSVDEngineGTest
participant SVD as vnl_svd

CMake->>Driver: add itkVnlSVDEngineGTest.cxx
Driver->>Test: run VnlSVDEngine test cases
Test->>SVD: construct from real/complex matrices
SVD-->>Test: inverse/solve/rank/nullspace/recompose results
Test->>Test: assert residuals and singular values
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 CMake as ITKCommon CMake
participant Driver as ITKCommonGTests
participant Test as itkVnlSVDEngineGTest
participant SVD as vnl_svd

CMake->>Driver: add itkVnlSVDEngineGTest.cxx
Driver->>Test: run VnlSVDEngine test cases
Test->>SVD: construct from real/complex matrices
SVD-->>Test: inverse/solve/rank/nullspace/recompose results
Test->>Test: assert residuals and singular values
Loading

Reviews (1): Last reviewed commit: "ENH: Add vnl_svd engine coverage as itkV..." | Re-trigger Greptile

@hjmjohnson

Copy link
Copy Markdown
Member Author

Downstream forest validation — SVD engine swap is invisible to consumers

Validated this change against 13 downstream ITK consumers built on top of f386a36bbe (this PR's head). All 13 build clean and every SVD code path with test coverage passes — 58/58 SVD-path tests, 0 regressions, 0 sign flips.

Build matrix — 13 consumers vs. this PR's ITK
Consumer Build SVD exposure
ANTs ✅ built Wahba/Kabsch rotation, JointFusion pinverse, tensor decompose
BRAINSTools ✅ built rotation orthonormalization, DTI recon, DWI gradients
elastix ✅ built thin-plate-spline svd.inverse, PCA metrics
SimpleITK ✅ built wraps LandmarkBasedTransformInitializer
c3d ✅ built Procrustes U·Vᵀ, affine solve, GLM pinverse
Plastimatch ✅ built RBF svd.solve, sphere/plane fit
OpenIGTLink ✅ built none
OpenIGTLinkIO ✅ built none
vtkAddon ✅ built none
IGSIO ✅ built spin-calibration Procrustes V·Uᵀ
PlusLib ✅ built phantom/probe calibration, plane nullvector
Slicer ✅ built fiducial registration, DTI FS-affine inverse
SlicerExtensions ✅ built none (descriptor bundle)

BRAINSTools SuperBuild reached [22/22] Completed and installed every binary; a harness rc=1 was traced to a post-build TBB link step, not a compile/link error.

Functional test results — SVD code paths exercised
Tool Tests Result SVD path proven
BRAINSTools 28 / 28 ✅ pass SVD orthonormalization, constellation aligner, gradient-sign preservation
ANTs 10 / 10 ✅ pass rotation registration (Wahba V·Uᵀ) + SyN-with-time
Plastimatch 18 / 18 ✅ pass Wendland + Gauss RBF svd.solve
elastix 2 / 2 ✅ pass thin-plate-spline svd.inverse
PlusLib via matrix ✅ pass plane/sphere/phantom fits — assertions sign-invariant by design
Slicer via matrix ✅ pass fiducial registration, ResampleDTIVolume baselines
Why the risk is bounded
  • Every asserted SVD path is sign-invariant. Tested call sites consume SVD as an intermediate — inverse, solve, pinverse, U·Vᵀ rotation, reconstruction — or compare registration output against a tolerance. None assert on raw singular-vector signs, the only quantity Eigen and LINPACK legitimately differ on.
  • The sharp edge is tested — with fabs(). The shared RANSAC PlaneParametersEstimator::nullvector() is byte-identical in Plastimatch and PlusLib. PlusLib's PlaneEstimationTest asserts fabs(dot(known, computed)) > 0.9962, so a flipped normal still passes — the exact invariance the swap relies on, pre-encoded by a domain expert.
  • "Untested" residue is upstream, not a forest gap. The few raw-vector paths without tests (ANTs tensor-decompose / surface-curvature / SCCAN, BRAINSTools' disabled DTI-recon add_test, elastix PCA) have no assertion tests upstream — they live in example / CLI / diagnostic code.
  • Accuracy proxy agrees. Eigen-vs-LINPACK on identical matrices: sign-invariant operations agree to ≤1.7e-13 (below every test tolerance); raw U/V differ by sign only. The functional results confirm the proxy end-to-end.

@hjmjohnson hjmjohnson self-assigned this Jul 6, 2026
@hjmjohnson hjmjohnson requested a review from dzenanz July 6, 2026 14:49
@dzenanz dzenanz removed their request for review July 6, 2026 15:56
@dzenanz

dzenanz commented Jul 6, 2026

Copy link
Copy Markdown
Member

I can't devote time to carefully reviewing this.

@hjmjohnson hjmjohnson merged commit 70cc06a into InsightSoftwareConsortium:main Jul 6, 2026
22 checks passed
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 type:Coverage Code coverage impacts 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.

3 participants