Skip to content

Commit f0aefa8

Browse files
committed
fix: raise LinAlgError on non-finite input, symmetrize before Cholesky
Addresses an adversarial review of the previous commit. 1. MAJOR regression, now fixed. scipy's cho_factor/cho_solve default to check_finite=True and raise ValueError -- not LinAlgError -- on a NaN/inf matrix. Both callers (inversion_plots.py:169 and :397) catch only LinAlgError, and the CSV writer's docstring explicitly promises that a failure there may not abort the enclosing model-fit. So a NaN-contaminated curvature matrix would have killed a fit that previously wrote a nan column and continued. Verified before and after: the old code returned [nan, nan]; the new code raised builtins.ValueError past the guards. Fixed by checking finiteness explicitly and raising LinAlgError with a message naming the cause, rather than by broadening the callers' except clauses -- the property's contract is now "raises LinAlgError for any input that has no covariance", which holds downstream too. check_finite=False is then passed to scipy, so the explicit check costs nothing net. 2. cho_factor reads only the upper triangle, so an asymmetric input was silently inverted as though its lower triangle matched. Verified: for [[2.0, 0.5], [0.1, 2.0]] it gave diag 0.5333 against the true inverse's 0.5063. The output symmetrization does not address this -- it symmetrizes the result, not the input -- so the input is now symmetrized too. curvature_reg_matrix is F + H and symmetric by construction, so this is defensive, but the silent wrong answer is not an acceptable failure mode. 3. Softened the "no value change" claim for reconstruction_noise_map. The two forms are algebraically identical but only numerically equivalent, since the covariance is now formed by Cholesky rather than LU: ~7e-15 relative at cond 1e3 rising to ~4e-5 at cond 1e13. Neither is the more correct result. 4. Tests. The symmetry test was tautological -- 0.5 * (C + C.T) is bitwise symmetric for any C, so it could only fail if the line were deleted. It now also asserts accuracy against an exactly-constructed ground truth (A = Q diag(w) Q.T), with the tolerance set from the measured error (~3e-9 at cond 1e9) and a comment recording that np.linalg.inv is marginally more accurate on that matrix, not less. Added tests for the non-finite raise and the asymmetric-input symmetrization. Fixed a stale test name that said "asymmetric_curvature_reg_matrix" over a symmetric matrix. Downstream check for the value-changing deprecated alias: no consumer of reconstruction_noise_map_with_covariance exists in PyAutoGalaxy (3ca31bf), PyAutoLens (87e5827) or autolens_workspace -- zero hits for "with_covariance" in any of them. reconstruction_noise_map IS used, in autolens_workspace source_science.py scripts computing signal_to_noise_map = reconstruction / reconstruction_noise_map, and its values are unchanged. Full suite: 1152 passed, 1 skipped. The 3 test_transformer.py pynufft failures are pre-existing on a6b07cd and unrelated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133X4XhMV91SFjzV2mK4Ejh
1 parent d91500d commit f0aefa8

2 files changed

Lines changed: 73 additions & 5 deletions

File tree

autoarray/inversion/inversion/abstract.py

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,14 @@ def reconstruction_covariance_matrix(self) -> np.ndarray:
862862
`cond ~ 1e12`, against ~3e-16 for the Cholesky solve), while a covariance matrix is symmetric by
863863
definition.
864864
865+
Every failure mode raises `LinAlgError`, including a non-finite `curvature_reg_matrix`. That case is
866+
checked explicitly because scipy would otherwise raise `ValueError`, which the callers -- here and
867+
downstream -- do not catch; the previous implementation returned a silently all-`NaN` matrix instead.
868+
869+
The matrix is symmetrized on input. `cho_factor` reads only the upper triangle, so an asymmetric input
870+
would otherwise be inverted as though its lower triangle matched, silently and with no diagnostic.
871+
`curvature_reg_matrix` is `F + H` and symmetric by construction, so this is defensive only.
872+
865873
This property is NumPy-only: the input is coerced with `np.asarray`, so a JAX `curvature_reg_matrix`
866874
forces a device-to-host transfer. It is a post-fit diagnostic, not part of the likelihood, so it is not on
867875
the JIT path.
@@ -874,8 +882,21 @@ def reconstruction_covariance_matrix(self) -> np.ndarray:
874882

875883
matrix = np.asarray(self.curvature_reg_matrix)
876884

885+
if not np.isfinite(matrix).all():
886+
raise np.linalg.LinAlgError(
887+
"The curvature_reg_matrix contains non-finite entries (NaN or inf), so the reconstruction "
888+
"covariance is undefined. Raised as LinAlgError so the plotting and CSV callers, which guard "
889+
"on LinAlgError, degrade gracefully rather than aborting the model-fit."
890+
)
891+
892+
# cho_factor reads only the upper triangle; symmetrize so an asymmetric input cannot be silently
893+
# inverted as though its lower triangle matched its upper.
894+
matrix = 0.5 * (matrix + matrix.T)
895+
877896
covariance = cho_solve(
878-
cho_factor(matrix), np.eye(matrix.shape[0], dtype=matrix.dtype)
897+
cho_factor(matrix, check_finite=False),
898+
np.eye(matrix.shape[0], dtype=matrix.dtype),
899+
check_finite=False,
879900
)
880901

881902
# cho_solve is accurate but not bitwise symmetric; a covariance matrix is symmetric by definition.
@@ -925,6 +946,12 @@ def reconstruction_noise_map(self):
925946
It is computed as the square root of the diagonal of `reconstruction_covariance_matrix`, which is the
926947
inverse of the same matrix used to solve for the reconstruction via the linear inversion.
927948
949+
This previously took the diagonal of an elementwise-square-rooted matrix. The two are algebraically
950+
identical -- `np.sqrt` is elementwise, so it commutes with taking the diagonal -- but only numerically
951+
equivalent, since the covariance is now formed by Cholesky rather than LU. The difference is
952+
conditioning-limited roundoff, measured at ~7e-15 relative at `cond ~ 1e3` rising to ~4e-5 at
953+
`cond ~ 1e13`; neither result is the more correct one.
954+
928955
Returns
929956
-------
930957
The noise-map of the reconstruction as a one dimensional ndarray, which does not account for the covariance

test_autoarray/inversion/inversion/test_abstract.py

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -678,7 +678,7 @@ def test__log_det_method__slogdet_is_finite_where_cholesky_fails_on_non_positive
678678
assert result == pytest.approx(np.linalg.slogdet(matrix)[1], 1.0e-8)
679679

680680

681-
def test__reconstruction_noise_map__asymmetric_curvature_reg_matrix__correct_diagonal_noise_values():
681+
def test__reconstruction_noise_map__correct_diagonal_noise_values():
682682
curvature_reg_matrix = np.array([[1.0, 1.0, 1.0], [1.0, 2.0, 1.0], [1.0, 1.0, 3.0]])
683683

684684
inversion = aa.m.MockInversion(curvature_reg_matrix=curvature_reg_matrix)
@@ -713,20 +713,61 @@ def test__reconstruction_covariance_matrix__off_diagonals_are_finite_and_negativ
713713
assert covariance == pytest.approx(np.linalg.inv(curvature_reg_matrix), 1.0e-8)
714714

715715

716-
def test__reconstruction_covariance_matrix__is_exactly_symmetric():
717-
"""A covariance matrix is symmetric by definition; `np.linalg.inv` drifts out of symmetry."""
716+
def test__reconstruction_covariance_matrix__is_accurate_and_symmetric_when_ill_conditioned():
717+
"""
718+
Ground truth is exact by construction: for `A = Q diag(w) Q.T` the inverse is `Q diag(1/w) Q.T`.
719+
720+
The symmetry half only guards the symmetrization line -- `0.5 * (C + C.T)` is bitwise symmetric for any C --
721+
so the accuracy assertion against the constructed truth is what tests the factorization itself.
722+
"""
718723
rng = np.random.default_rng(1234)
719724
q, _ = np.linalg.qr(rng.standard_normal((25, 25)))
720-
curvature_reg_matrix = (q * np.logspace(0, 9, 25)) @ q.T
725+
eigenvalues = np.logspace(0, 9, 25)
726+
727+
curvature_reg_matrix = (q * eigenvalues) @ q.T
721728
curvature_reg_matrix = 0.5 * (curvature_reg_matrix + curvature_reg_matrix.T)
722729

730+
covariance_true = (q * (1.0 / eigenvalues)) @ q.T
731+
723732
inversion = aa.m.MockInversion(curvature_reg_matrix=curvature_reg_matrix)
724733

725734
covariance = inversion.reconstruction_covariance_matrix
726735

736+
# cond ~ 1e9, so the achievable accuracy is eps * cond ~ 2e-7; the measured error is ~3e-9. This is not a
737+
# claim that Cholesky beats LU here -- it does not, `np.linalg.inv` measures ~7e-10 on this matrix.
738+
assert covariance == pytest.approx(covariance_true, abs=1.0e-7)
727739
assert covariance == pytest.approx(covariance.T, abs=1.0e-15)
728740

729741

742+
def test__reconstruction_covariance_matrix__asymmetric_input_is_symmetrized_not_silently_upper_triangle():
743+
"""
744+
`cho_factor` reads only the upper triangle, so an asymmetric input would be inverted as though its lower
745+
triangle matched its upper -- silently, and differing from the true inverse.
746+
"""
747+
curvature_reg_matrix = np.array([[2.0, 0.5], [0.1, 2.0]])
748+
symmetrized = 0.5 * (curvature_reg_matrix + curvature_reg_matrix.T)
749+
750+
inversion = aa.m.MockInversion(curvature_reg_matrix=curvature_reg_matrix)
751+
752+
assert inversion.reconstruction_covariance_matrix == pytest.approx(
753+
np.linalg.inv(symmetrized), 1.0e-8
754+
)
755+
756+
757+
def test__reconstruction_covariance_matrix__non_finite_matrix_raises_lin_alg_error():
758+
"""
759+
scipy raises `ValueError` on a non-finite matrix, which the plotting and CSV callers do not catch -- they
760+
guard on `LinAlgError`. The CSV writer explicitly promises not to abort the enclosing model-fit, so the
761+
non-finite case is converted rather than allowed to escape.
762+
"""
763+
curvature_reg_matrix = np.array([[1.0, np.nan], [np.nan, 2.0]])
764+
765+
inversion = aa.m.MockInversion(curvature_reg_matrix=curvature_reg_matrix)
766+
767+
with pytest.raises(np.linalg.LinAlgError, match="non-finite"):
768+
inversion.reconstruction_covariance_matrix
769+
770+
730771
def test__reconstruction_noise_map__is_sqrt_of_covariance_diagonal():
731772
"""
732773
The invariant, asserted directly rather than via hand-computed values.

0 commit comments

Comments
 (0)