ENH: Deprecate the vnl_svd family for the Eigen-backed itk::Math::SVD#6564
ENH: Deprecate the vnl_svd family for the Eigen-backed itk::Math::SVD#6564hjmjohnson wants to merge 4 commits into
Conversation
|
Scope update (superseded): the |
6707e4c to
76036f9
Compare
76036f9 to
619512b
Compare
619512b to
520c2f2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Extend the Eigen-backed itk::Math::SVD helpers so every vnl_svd / vnl_matrix_inverse call site in ITK has a direct replacement: pseudo-inverse, solve, rank, recompose, null vector, and the WellCondition and DeterminantMagnitude predicates. Route the eigenvector sign convention through itkEigenDecompositionSignConvention so results are backend-stable.
Exercise the extended itk::Math::SVD surface (pseudo-inverse, solve, rank, WellCondition, DeterminantMagnitude) and cross-check it against the vnl_svd result on well- and ill-conditioned inputs.
520c2f2 to
7c01e6f
Compare
…h::SVD Replace direct vnl_svd, vnl_svd_fixed and vnl_matrix_inverse use across the transforms, diffusion-tensor reconstruction, image moments, label-map filters, membership functions, Mahalanobis metrics, the Quasi-Newton optimizer, NIfTI and MINC IO and the classifier estimators with the Eigen-backed itk::Math::SVD API, including itk::Matrix::GetInverse. Where a widely included header (itkTransform, itkMatrix, the affine transforms) previously pulled a vnl_svd* header transitively, keep that include reachable under #if !defined(ITK_LEGACY_REMOVE) && !defined(ITK_FUTURE_LEGACY_REMOVE) so downstream code that relied on the transitive include still compiles during the deprecation window.
Add the three-state deprecation guard to vnl_svd, vnl_svd_fixed, vnl_svd_economy, vnl_matrix_inverse and vnl_solve_qp, pointing callers at the Eigen-backed itk::Math::SVD: silent by default, a #warning under ITK_LEGACY_REMOVE and a hard #error under ITK_FUTURE_LEGACY_REMOVE. Build-gate the vnl_svd family and the LINPACK svdc netlib sources out of the ITK_FUTURE_LEGACY_REMOVE build so the dead engine no longer compiles once the APIs are gone, and move the vnl algo tests off the deprecated spellings.
7c01e6f to
232a2ad
Compare
|
Fixed the |
|
Is the recommendation moving forward going to be to directly use Eigen or use new ITK interfaces? |
I would say to use the ITK interfaces. A few (or perhaps many now) years ago, at a NA-MIC meeting, several people were lamenting the fact that VXL was exposed to end-users as part of the ITK API, and how that decision both increased the maintenance burden and limited how we could improve ITK. I am trying to avoid that trap with Eigen. Internally, we can use Eigen directly, but we should provide an ITK API wrapper that does not expose Eigen to consumers of ITK. This will allow future algorithmic changes to be made independent of what Eigen does. Judicious use of itk::Math::detail namespaces where Eigen interfaces provide performance benefits are used to indicate that, while they are part of the 'technical' public API, conceptually we do not consider them part of the public API, and we do not make any guarantees about backward compatibility for those details. |
I had thought that comment referenced the classes that were derived from VNL object, along with the the operator that operated on VNL objects. |
@blowekamp Do you see this as a step in the wrong direction, or are you concerned about the approach? I'd like to understand your view on the correct approach. Do you think a Zoom call would help? My approach/motivation: I was trying to replace vnl_ (backed by pre-1999 FORTRAN) -> itk::Math:: (backed by Eigen). In most cases, this provides 2 wins: 1) Performance (limited at the moment, Eigen unlocks other performance improvements that will need to be revisited later), and 2) Almost always better numerical accuracy. Also, Eigen is having a huge (Nvidia-backed) engineering modernization effort going on right now that will hopefully filter into ITK for free. |
Deprecates the
vnl_svdfamily (vnl_svd,vnl_svd_fixed,vnl_svd_economy,vnl_matrix_inverse) in favor of the Eigen-backeditk::Math::SVD, migrates every in-tree consumer, and build-gates the LINPACKsvdcengine out ofITK_FUTURE_LEGACY_REMOVE. Theitk::Math::SolveSymmetric/InverseSymmetric(LDLT) work that previously lived here is split out to #6565 so the two review independently.Commit structure (4 thematic commits)
itk::Math::SVD— pseudo-inverse, solve, rank, recompose, null-vector,WellCondition,DeterminantMagnitude; eigenvector sign routed throughitkEigenDecompositionSignConventionfor backend-stable results.vnl_svdon well- and ill-conditioned inputs.itk::Matrix::GetInverse). Widely-included headers keep the transitivevnl_svd*include reachable under#if !defined(ITK_LEGACY_REMOVE) && !defined(ITK_FUTURE_LEGACY_REMOVE)so downstream code that relied on it still compiles during the deprecation window.vnl_svdfamily with the three-state guard (silent default /#warningunderITK_LEGACY_REMOVE/#errorunderITK_FUTURE_LEGACY_REMOVE) and build-gate the LINPACKsvdcsources.Downstream validation (forest)
Built against this branch's tip in the ITK downstream forest: ITK, ANTs, BRAINSTools, c3d, SimpleITK all compile clean. The one real regression found — ANTs
antsAffineInitializerrelying on a transitivevnl_svd_fixed.hviaitkTransform.hxx— is fixed by the guarded transitive includes in commit 3. No consumer hit anyvnl_svd/vnl_matrix_inverse/itk::Math::SVDcompile error. (c3d and SimpleITK additionally needed their own upstream ITK6-migration PRs, unrelated to this change.)