COMP: Use itk::Math::MatrixExponential instead of removed vnl_matrix_exp#1452
COMP: Use itk::Math::MatrixExponential instead of removed vnl_matrix_exp#1452hjmjohnson wants to merge 1 commit into
Conversation
2e19386 to
c33e80a
Compare
|
Performance impact of migrating AffineLogTransform from Per-call timing + net AffineLogTransform impact
Per
The d² Numerical equivalence + accuracyFor near-identity log-domain matrices the absolute output difference between the two methods is ~1e-12 — far below any registration tolerance, so registration results are unchanged. Eigen (Higham scaling-and-squaring) is also ~2–3 orders of magnitude more accurate than vnl's Taylor series on the This PR depends on |
|
Thanks Hans. Would this compile for both ITK 5 and 6, when we add something like |
VXL removed core/vnl/vnl_matrix_exp.h, breaking AffineLogTransform's include of <vnl/vnl_matrix_exp.h> (InsightSoftwareConsortium/ITK#6452). Switch to itk::Math::MatrixExponential (itkMatrixExponential.h), an ITK-supported Eigen-backed replacement using Higham scaling-and-squaring. elastix's minimum is ITK 5.4.1, which lacks itkMatrixExponential.h, so guard the include and both call sites on __has_include and fall back to vnl_matrix_exp for older ITK. ITK#6454 adds the function and restores the vnl_matrix_exp shim. Call-site argument types (vnl_matrix_fixed, vnl_matrix) are unchanged.
c33e80a to
5aeef7f
Compare
|
@N-Dekker Good catch — yes, and a guard is necessary: elastix's minimum is ITK 5.4.1 ( Why __has_include rather than #if ITK_VERSION_MAJOR >= 6
#ifndef ELX_HAS_ITK_MATRIX_EXPONENTIAL
# if __has_include("itkMatrixExponential.h")
# define ELX_HAS_ITK_MATRIX_EXPONENTIAL 1
# else
# define ELX_HAS_ITK_MATRIX_EXPONENTIAL 0
# endif
#endifVerified both branches compile locally: the Eigen path builds the full elastix/transformix against ITK main; the vnl fallback (forced via |
|
|
||
| #include <vnl/vnl_matrix_exp.h> | ||
| // itk::Math::MatrixExponential exists in ITK >= 6 (ITK#6454); ITK 5.4.x lacks it, so fall back to vnl_matrix_exp. | ||
| #ifndef ELX_HAS_ITK_MATRIX_EXPONENTIAL |
There was a problem hiding this comment.
Thanks Hans. I guess this #ifndef ELX_HAS_ITK_MATRIX_EXPONENTIAL is there to allow users to overrule the automatic detection of "itkMatrixExponential.h", right? So that they can explicitly choose between "itkMatrixExponential.h" and <vnl/vnl_matrix_exp.h>. Is that indeed the idea behind this initial #ifdef?
(Otherwise we might consider #undef ELX_HAS_ITK_MATRIX_EXPONENTIAL at the end of the hxx file.)
N-Dekker
left a comment
There was a problem hiding this comment.
Approved unconditionally, but feel free to comment on my question about the initial #ifndef ELX_HAS_ITK_MATRIX_EXPONENTIAL 😃
|
@hjmjohnson Maybe we should still check if it won't break our regression tests, as it might yield slightly different result, right? But for now, I guess we have no choice, because without this PR, elastix + ITK6 won't build anymore 🤷 |
@hjmjohnson This sounds very promising. Can you possibly share the benchmark code? |
VXL removed
core/vnl/vnl_matrix_exp, whichAffineLogTransformincluded via<vnl/vnl_matrix_exp.h>(InsightSoftwareConsortium/ITK#6452). This migrates the two call sites toitk::Math::MatrixExponential, an ITK-supported Eigen-backed replacement.Change
Call-site argument types (
vnl_matrix_fixed,vnl_matrix) are unchanged; the new function provides matching overloads. BothAffineLogTransformandAffineLogStackTransformcomponents rebuild clean against the new header.