Skip to content

fix(linalg): preserve f64 SVD determinants and integrate into Umeyama#799

Open
AnilKumarSingh9856 wants to merge 8 commits intokornia:mainfrom
AnilKumarSingh9856:feat/clean-umeyama-f64
Open

fix(linalg): preserve f64 SVD determinants and integrate into Umeyama#799
AnilKumarSingh9856 wants to merge 8 commits intokornia:mainfrom
AnilKumarSingh9856:feat/clean-umeyama-f64

Conversation

@AnilKumarSingh9856
Copy link
Contributor

@AnilKumarSingh9856 AnilKumarSingh9856 commented Mar 9, 2026

📝 Description

⚠️ Issue Link Required: This PR must be linked to an approved and assigned issue. See Contributing Guide for details.

Fixes #452 (Also supersedes closed PR #556)

Important:

  • Ensure you are assigned to the linked issue before submitting this PR
  • This PR should strictly implement what the linked issue describes
  • Do not include changes beyond the scope of the linked issue

🛠️ Changes Made

  • Removed nalgebra from Umeyama: Completely replaced the dual-SVD fallback in umeyama.rs with the internal svd3_f64 engine.
  • Fixed SVD Determinant Preservation: Patched a critical silent bug in impl_f64::sort_singular_values within svd.rs. Swapping columns of $U$ and $V$ previously flipped their determinants from +1 to -1 (turning pure rotations into reflections). They are now properly negated during swaps to preserve orthonormality.
  • Maintained Legacy Reflection: Kept the downstream-compatible reflection handling (r.z_axis = -r.z_axis) in umeyama to prevent regressions in existing EPnP expectations.
  • Restored Strict Tolerances: Because the SVD math is now highly accurate and determinant-preserving, I completely reverted epnp.rs and fundamental.rs back to their strict downstream test tolerances (1e-6 and 5e-5).

🧪 How Was This Tested?

  • Unit Tests: Ran cargo test --workspace --all-features. Verified all 247 kornia-algebra tests and 54 kornia-3d tests pass flawlessly.
  • Manual Verification: Verified that the strict tolerances in downstream algorithms (EPnP, Fundamental Matrix) pass without the precision drift seen in previous PRs. Formatted and linted with cargo clippy -D warnings.
  • Performance/Edge Cases: Ran cargo bench -p kornia-algebra. The svd3_f64 implementation actually showed a ~2% performance improvement. It robustly handles reflection cases and repeated singular values.

🕵️ AI Usage Disclosure

Check one of the following:

  • 🟢 No AI used.
  • 🟡 AI-assisted: I used AI for boilerplate/refactoring but have manually reviewed and tested every line.
  • 🔴 AI-generated: (Note: These PRs may be subject to stricter scrutiny or immediate closure if the logic is not explained).

🚦 Checklist

  • I am assigned to the linked issue (required before PR submission)
  • The linked issue has been approved by a maintainer
  • This PR strictly implements what the linked issue describes (no scope creep)
  • I have performed a self-review of my code (no "ghost" variables or hallucinations).
  • My code follows the existing style guidelines of this project.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • (Optional) I have attached screenshots/recordings for UI changes.

💭 Additional Context

To @cjpurackal and @edgarriba: This revives the work from #556. You were 100% right previously to question why the f64 implementation was struggling with accuracy in the downstream EPnP tests.

I traced the root cause: the standalone sort_singular_values function was swapping columns without negating them, introducing a silent reflection error that caused the precision drift downstream. With that math fixed, the internal svd3_f64 is now perfectly accurate, allowing us to drop nalgebra entirely and restore the strictest test tolerances.

Copilot AI review requested due to automatic review settings March 9, 2026 10:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves rigid alignment accuracy by switching Umeyama’s SVD path to the internal svd3_f64 implementation and updates the f64 SVD implementation to better preserve matrix orientation when singular vectors are reordered.

Changes:

  • Update svd3_f64 internals (Jacobi tolerance, symmetric initialization, QR flow) and add an orientation-preserving singular-vector sorting routine.
  • Replace nalgebra-based SVD usage in umeyama with svd3_f64 and compute centroids/covariance in f64 before casting outputs to f32.
  • Add/adjust Umeyama tests and rename an f64 SVD degeneracy test.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
crates/kornia-algebra/src/linalg/svd.rs Fixes/updates f64 SVD implementation and introduces a new public singular-value sorting API intended to preserve orientation.
crates/kornia-algebra/src/linalg/rigid.rs Replaces Umeyama’s nalgebra SVD with internal svd3_f64, performing intermediate math in f64 and adding tests.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copy link
Collaborator

@sidd-27 sidd-27 left a comment

Choose a reason for hiding this comment

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

Technical Review

Excellent Work - Strong Technical Foundation

This is a high-quality fix that addresses a fundamental mathematical issue in the SVD implementation. The changes are well-reasoned and properly tested.

🔬 Key Technical Improvements

  1. Critical SVD Bug Fix: The fix in sort_singular_values where swapped columns are now negated (-b_y, -v_y) is mathematically correct. Previously, column swaps were flipping determinants from +1 to -1, converting pure rotations into reflections. This was a silent but serious numerical accuracy issue.

  2. nalgebra Removal: Using internal svd3_f64 throughout eliminates the external dependency and gives full control over numerical precision. The manual covariance computation using raw f64 prevents precision loss during intermediate operations.

  3. Comprehensive Testing: The new test coverage is excellent:

    • Translation-only case
    • Rotation + translation
    • Complex 3D transform
    • Orientation preservation test - this is particularly valuable for catching the determinant issue

📊 Code Quality

Strengths:

  • Clean f64 → f32 casting only at the output boundary
  • Manual covariance computation avoids wrapper type limitations
  • Proper error handling with descriptive error types
  • Well-documented mathematical steps

Minor Suggestions:

  • Consider adding a comment in sort_singular_values explaining why columns are negated during swaps (determinant preservation)
  • The Symmetric3x3::from_mat3x3 could use a brief comment about the direct assignment approach

🚨 CI Issues to Address

The failing checks need attention:

  • Validate PR Requirements: Likely a workflow issue
  • py3.14t-linux: Python test failure - please investigate if this change affects Python bindings

🎯 Impact Assessment

This change should significantly improve numerical stability in:

  • EPnP pose estimation
  • Fundamental matrix computation
  • Any downstream algorithm depending on Umeyama

The restoration of strict tolerances (1e-6, 5e-5) in downstream tests validates that the precision issues are resolved.

Recommendation

Approve after CI fixes - This is exactly the kind of mathematical rigor the codebase needs. The SVD determinant preservation fix alone makes this a critical improvement.

Great work on tracking down this subtle but important numerical issue! 🎉

@AnilKumarSingh9856
Copy link
Contributor Author

@sidd-27, Thanks for the thorough review and the positive feedback on the mathematical rigor!

I have addressed your suggestions:

Documentation: Added explicit comments in sort_singular_values explaining the determinant preservation math and documented the direct assignment approach in Symmetric3x3::from_mat3x3.

CI Fixes: I've updated the PR description to satisfy the 'Validate PR Requirements' check. Regarding the py3.14t-linux failure, it appears to be a flake in the experimental 3.14-dev runner unrelated to these changes, as the stable Python tests (3.8–3.13) are all passing.

The restoration of strict tolerances in the downstream tests confirms the fix is working as intended. Ready for a final look!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make kornia-linalg::umeyama use kornia-linalg::svd3 instead of naglebra's SVD.

3 participants