Skip to content

Commit

Permalink
Fix numerical issue on SVD with near-identity matrix (#1369)
Browse files Browse the repository at this point in the history
* fix: Normalize the column once more

The column may not be normalized if the `factor` is on a scale of 1e-40.
Possibly, f32 just runs out of precision.

There is likely a better solution to the problem.

* chore: Add test that fails before fix

* chore: add comment providing details on the householder fix.

* chore: rename regression test

---------

Co-authored-by: Sébastien Crozet <sebcrozet@dimforge.com>
  • Loading branch information
Vollkornaffe and sebcrozet committed Mar 28, 2024
1 parent 749a9fe commit c475c40
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 1 deletion.
11 changes: 11 additions & 0 deletions src/linalg/householder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ pub fn reflection_axis_mut<T: ComplexField, D: Dim, S: StorageMut<T, D>>(

if !factor.is_zero() {
column.unscale_mut(factor.sqrt());

// Normalize again, making sure the vector is unit-sized.
// If `factor` had a very small value, the first normalization
// (dividing by `factor.sqrt()`) might end up with a slightly
// non-unit vector (especially when using 32-bits float).
// Decompositions strongly rely on that unit-vector property,
// so we run a second normalization (that is much more numerically
// stable since the norm is close to 1) to ensure it has a unit
// size.
let _ = column.normalize_mut();

(-signed_norm, true)
} else {
// TODO: not sure why we don't have a - sign here.
Expand Down
27 changes: 26 additions & 1 deletion tests/linalg/eigen.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use na::DMatrix;
use na::{DMatrix, Matrix3};

#[cfg(feature = "proptest-support")]
mod proptest_tests {
Expand Down Expand Up @@ -116,6 +116,31 @@ fn symmetric_eigen_singular_24x24() {
);
}

// Regression test for #1368
#[test]
fn very_small_deviation_from_identity_issue_1368() {
let m = Matrix3::<f32>::new(
1.0,
3.1575704e-23,
8.1146196e-23,
3.1575704e-23,
1.0,
1.7471054e-22,
8.1146196e-23,
1.7471054e-22,
1.0,
);

for v in m
.try_symmetric_eigen(f32::EPSILON, 0)
.unwrap()
.eigenvalues
.into_iter()
{
assert_relative_eq!(*v, 1.);
}
}

// #[cfg(feature = "arbitrary")]
// quickcheck! {
// TODO: full eigendecomposition is not implemented yet because of its complexity when some
Expand Down

0 comments on commit c475c40

Please sign in to comment.