Skip to content

Commit 1693307

Browse files
acgetchelloz-agent
andcommitted
docs: strengthen LDLT symmetry precondition and add is_symmetric API
LDLT factorization assumes the input matrix is symmetric, but the contract was only implied by one sentence in the module and struct docs. Asymmetric inputs silently produce mathematically meaningless factorizations in release builds (the `debug_assert_symmetric` check is compiled out), and callers had no supported way to validate symmetry up front. Documentation: - `Matrix::ldlt`: add a prominent `# Preconditions` section spelling out the contract, the debug-vs-release split, and pointers to the new `is_symmetric` / `first_asymmetry` predicates and to `lu()` as the fallback for inputs that may not be symmetric at all. - `src/ldlt.rs`: promote the precondition into a module-level and struct-level `# Preconditions` section with back-links. - `README.md`: add a warning call-out under the LDLT example linking the new predicates. New public API (`Matrix<D>`): - `is_symmetric(&self, rel_tol: f64) -> bool` — infallible predicate sharing the `|A[r][c] - A[c][r]| <= rel_tol * max(1, inf_norm)` convention used internally by LDLT. - `first_asymmetry(&self, rel_tol: f64) -> Option<(usize, usize)>` — returns the lexicographically first off-diagonal pair that violates symmetry, or `None` for symmetric matrices. Used by the debug-build check for pinpointed panic messages. - Both `debug_assert!(rel_tol >= 0.0)`, matching `lu(tol)` / `ldlt(tol)`. - NaN off-diagonals are explicitly reported as asymmetric. Refactor: - `debug_assert_symmetric` now delegates to `first_asymmetry`, so the runtime check and the documented contract share one implementation and cannot drift apart. Tests: - Dimension-generic (D=2..=5): identity, zero, `A = M + Mᵀ`, perturbed off-diagonal asymmetric, NaN off-diagonal asymmetric. - Scalar: tolerance scaling with inf_norm, lexicographic-first pair on D=3, debug-only panic on negative `rel_tol`. - `#[cfg(debug_assertions)] #[should_panic]` test for the LDLT debug-build panic still passes against the refactored assertion. No functional change in release builds of existing APIs. Tests: `just ci` — 123 lib, 26 doc, 284 exact-feature, 101 Python, all examples, all linters/validators. Closes #84 Co-Authored-By: Oz <oz-agent@warp.dev>
1 parent b60a734 commit 1693307

3 files changed

Lines changed: 273 additions & 11 deletions

File tree

README.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,17 @@ let det = a.ldlt(DEFAULT_SINGULAR_TOL).unwrap().det();
108108
assert!((det - 1.0).abs() <= 1e-12);
109109
```
110110

111+
> ⚠️ **LDLT precondition:** The input matrix must be **symmetric**. Symmetry is
112+
> verified by a `debug_assert!` in debug builds only; release builds silently accept
113+
> asymmetric inputs and produce a meaningless factorization. Pre-validate with
114+
> [`Matrix::is_symmetric`](https://docs.rs/la-stack/latest/la_stack/struct.Matrix.html#method.is_symmetric)
115+
> (or locate the offending pair with
116+
> [`Matrix::first_asymmetry`](https://docs.rs/la-stack/latest/la_stack/struct.Matrix.html#method.first_asymmetry))
117+
> when you cannot statically guarantee symmetry, or fall back to `lu()` if your
118+
> matrices may not be symmetric at all. See
119+
> [`Matrix::ldlt`](https://docs.rs/la-stack/latest/la_stack/struct.Matrix.html#method.ldlt)
120+
> for details.
121+
111122
## ⚡ Compile-time determinants (D ≤ 4)
112123

113124
`det_direct()` is a `const fn` providing closed-form determinants for D=0–4,

src/ldlt.rs

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,16 @@
33
//! This module provides a stack-allocated LDLT factorization (`A = L D Lᵀ`) intended for
44
//! symmetric positive definite (SPD) and positive semi-definite (PSD) matrices (e.g. Gram
55
//! matrices) without pivoting.
6+
//!
7+
//! # Preconditions
8+
//! The input matrix must be **symmetric**. This is a correctness contract, not a hint:
9+
//! the factorization algorithm reads only the lower triangle and implicitly assumes the
10+
//! upper triangle mirrors it. Symmetry is verified by a `debug_assert!` in debug builds
11+
//! only; in release builds an asymmetric input will silently produce a meaningless
12+
//! factorization. Callers who cannot statically guarantee symmetry should pre-validate
13+
//! with [`Matrix::is_symmetric`](crate::Matrix::is_symmetric) (or locate the offending
14+
//! pair with [`Matrix::first_asymmetry`](crate::Matrix::first_asymmetry)), or fall back
15+
//! to [`crate::Lu`] if their matrices are not guaranteed to be symmetric at all.
616
717
use core::hint::cold_path;
818

@@ -15,6 +25,16 @@ use crate::vector::Vector;
1525
/// This factorization is **not** a general-purpose symmetric-indefinite LDLT (no pivoting).
1626
/// It assumes the input matrix is symmetric and (numerically) SPD/PSD.
1727
///
28+
/// # Preconditions
29+
/// The source matrix passed to [`Matrix::ldlt`](crate::Matrix::ldlt) must be symmetric
30+
/// (`A[i][j] == A[j][i]` within rounding). Asymmetric inputs panic in debug builds via
31+
/// `debug_assert!` and are silently accepted in release builds — producing a
32+
/// mathematically meaningless factorization whose [`Self::det`] and [`Self::solve_vec`]
33+
/// results are wrong without any error being reported. Pre-validate with
34+
/// [`Matrix::is_symmetric`](crate::Matrix::is_symmetric) when the input cannot be
35+
/// statically guaranteed symmetric; see [`Matrix::ldlt`](crate::Matrix::ldlt) for further
36+
/// details and alternatives.
37+
///
1838
/// # Storage
1939
/// The factors are stored in a single [`Matrix`]:
2040
/// - `D` is stored on the diagonal.
@@ -193,17 +213,18 @@ impl<const D: usize> Ldlt<D> {
193213

194214
#[cfg(debug_assertions)]
195215
fn debug_assert_symmetric<const D: usize>(a: &Matrix<D>) {
196-
let scale = a.inf_norm().max(1.0);
197-
let eps = 1e-12 * scale;
198-
199-
for r in 0..D {
200-
for c in (r + 1)..D {
201-
let diff = (a.rows[r][c] - a.rows[c][r]).abs();
202-
debug_assert!(
203-
diff <= eps,
204-
"matrix must be symmetric (diff={diff}, eps={eps}) at ({r}, {c})"
205-
);
206-
}
216+
// Delegate to the public predicate so the runtime check and the documented
217+
// contract on `Matrix::ldlt` cannot drift apart. `first_asymmetry` is used
218+
// (rather than `is_symmetric`) so the panic message can name the offending
219+
// pair — which is invaluable for debugging.
220+
if let Some((r, c)) = a.first_asymmetry(1e-12) {
221+
let diff = (a.rows[r][c] - a.rows[c][r]).abs();
222+
let eps = 1e-12 * a.inf_norm().max(1.0);
223+
debug_assert!(
224+
false,
225+
"matrix must be symmetric (diff={diff}, eps={eps}) at ({r}, {c}); \
226+
pre-validate with Matrix::is_symmetric before calling ldlt"
227+
);
207228
}
208229
}
209230

@@ -433,4 +454,18 @@ mod tests {
433454
let err = ldlt.solve_vec(b).unwrap_err();
434455
assert_eq!(err, LaError::NonFinite { row: None, col: 1 });
435456
}
457+
458+
/// Verifies the symmetry precondition documented on [`Matrix::ldlt`] is
459+
/// enforced by `debug_assert_symmetric` in debug builds. The test is
460+
/// gated on `debug_assertions` so `cargo test --release` simply skips it
461+
/// (the assertion is compiled out in release builds — see the
462+
/// Preconditions section of `Matrix::ldlt`).
463+
#[cfg(debug_assertions)]
464+
#[test]
465+
#[should_panic(expected = "matrix must be symmetric")]
466+
fn debug_asymmetric_input_panics() {
467+
// a[0][1] = 2.0 but a[1][0] = -2.0 → clearly asymmetric.
468+
let a = Matrix::<3>::from_rows([[4.0, 2.0, 0.0], [-2.0, 5.0, 1.0], [0.0, 1.0, 3.0]]);
469+
let _ = a.ldlt(DEFAULT_SINGULAR_TOL);
470+
}
436471
}

src/matrix.rs

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,94 @@ impl<const D: usize> Matrix<D> {
152152
max_row_sum
153153
}
154154

155+
/// Returns `true` if the matrix is symmetric within a relative tolerance.
156+
///
157+
/// Two entries `self[r][c]` and `self[c][r]` are considered equal (for the
158+
/// purposes of symmetry) when
159+
/// `|self[r][c] - self[c][r]| <= rel_tol * max(1.0, self.inf_norm())`.
160+
/// This mirrors the predicate used internally by the debug-build symmetry
161+
/// check inside [`ldlt`](Self::ldlt), so callers can pre-validate matrices
162+
/// that may come from untrusted sources without relying on a debug-only
163+
/// panic.
164+
///
165+
/// Use [`first_asymmetry`](Self::first_asymmetry) to locate the first
166+
/// offending pair when this returns `false`.
167+
///
168+
/// # NaN / infinity handling
169+
/// Any NaN off-diagonal entry causes this predicate to return `false`
170+
/// (because `NaN <= eps` is `false`). A matrix whose [`inf_norm`](Self::inf_norm)
171+
/// is `+∞` can produce an `eps` of `+∞`, under which any finite asymmetry
172+
/// is tolerated — callers who need strict equality on infinite entries
173+
/// should validate finiteness separately.
174+
///
175+
/// # Panics
176+
/// In debug builds, panics if `rel_tol` is negative or NaN; in release
177+
/// builds these are silently treated as garbage-in garbage-out, matching
178+
/// the convention of [`lu`](Self::lu) and [`ldlt`](Self::ldlt).
179+
///
180+
/// # Examples
181+
/// ```
182+
/// use la_stack::prelude::*;
183+
///
184+
/// let a = Matrix::<2>::from_rows([[4.0, 2.0], [2.0, 3.0]]);
185+
/// assert!(a.is_symmetric(1e-12));
186+
///
187+
/// let b = Matrix::<2>::from_rows([[4.0, 2.0], [3.0, 3.0]]);
188+
/// assert!(!b.is_symmetric(1e-12));
189+
/// ```
190+
#[inline]
191+
#[must_use]
192+
pub fn is_symmetric(&self, rel_tol: f64) -> bool {
193+
self.first_asymmetry(rel_tol).is_none()
194+
}
195+
196+
/// Returns the indices `(r, c)` (with `r < c`) of the first off-diagonal
197+
/// pair that violates symmetry, or `None` if the matrix is symmetric
198+
/// within `rel_tol`.
199+
///
200+
/// Iteration order is row-major over the strict upper triangle, so the
201+
/// returned indices are the lexicographically smallest such pair. The
202+
/// predicate is the same as [`is_symmetric`](Self::is_symmetric):
203+
/// `|self[r][c] - self[c][r]| <= rel_tol * max(1.0, self.inf_norm())`.
204+
///
205+
/// # Panics
206+
/// In debug builds, panics if `rel_tol` is negative or NaN.
207+
///
208+
/// # Examples
209+
/// ```
210+
/// use la_stack::prelude::*;
211+
///
212+
/// let a = Matrix::<3>::from_rows([
213+
/// [1.0, 2.0, 0.0],
214+
/// [2.0, 4.0, 5.0],
215+
/// [0.0, 6.0, 9.0], // 6.0 breaks symmetry with a[1][2] = 5.0
216+
/// ]);
217+
/// assert_eq!(a.first_asymmetry(1e-12), Some((1, 2)));
218+
/// assert_eq!(Matrix::<3>::identity().first_asymmetry(1e-12), None);
219+
/// ```
220+
#[inline]
221+
#[must_use]
222+
pub fn first_asymmetry(&self, rel_tol: f64) -> Option<(usize, usize)> {
223+
debug_assert!(
224+
rel_tol >= 0.0,
225+
"rel_tol must be non-negative (got {rel_tol})"
226+
);
227+
let eps = rel_tol * self.inf_norm().max(1.0);
228+
for r in 0..D {
229+
for c in (r + 1)..D {
230+
let diff = (self.rows[r][c] - self.rows[c][r]).abs();
231+
// NaN is reported as asymmetric: a NaN entry contaminates `diff`,
232+
// and `diff > eps` alone would silently skip it because ordered
233+
// comparisons against NaN are always `false`.
234+
if diff.is_nan() || diff > eps {
235+
cold_path();
236+
return Some((r, c));
237+
}
238+
}
239+
}
240+
None
241+
}
242+
155243
/// Compute an LU decomposition with partial pivoting.
156244
///
157245
/// # Examples
@@ -185,11 +273,31 @@ impl<const D: usize> Matrix<D> {
185273
/// This is intended for symmetric positive definite (SPD) and positive semi-definite (PSD)
186274
/// matrices such as Gram matrices.
187275
///
276+
/// # Preconditions
277+
/// **The input matrix `self` must be symmetric** — that is, `self[i][j] == self[j][i]`
278+
/// (within rounding) for all `i`, `j`. This is a *correctness* precondition, not merely
279+
/// a performance hint.
280+
///
281+
/// - In **debug builds** a `debug_assert!` verifies symmetry via
282+
/// [`is_symmetric`](Self::is_symmetric) (relative tolerance scaled by the matrix's
283+
/// infinity norm) and panics if it fails.
284+
/// - In **release builds** the check is compiled out for performance. An asymmetric
285+
/// input will be accepted silently and produce a mathematically meaningless
286+
/// factorization — subsequent calls to [`Ldlt::det`] and [`Ldlt::solve_vec`] will
287+
/// return wrong results with no error.
288+
///
289+
/// Callers who cannot statically guarantee symmetry should pre-validate with
290+
/// [`is_symmetric`](Self::is_symmetric) (or locate the offending pair with
291+
/// [`first_asymmetry`](Self::first_asymmetry)) before calling `ldlt`. If you need a
292+
/// general-purpose factorization that tolerates non-symmetric inputs, use
293+
/// [`lu`](Self::lu) instead.
294+
///
188295
/// # Examples
189296
/// ```
190297
/// use la_stack::prelude::*;
191298
///
192299
/// # fn main() -> Result<(), LaError> {
300+
/// // Note the symmetric layout: a[0][1] == a[1][0] == 2.0.
193301
/// let a = Matrix::<2>::from_rows([[4.0, 2.0], [2.0, 3.0]]);
194302
/// let ldlt = a.ldlt(DEFAULT_SINGULAR_TOL)?;
195303
///
@@ -210,6 +318,9 @@ impl<const D: usize> Matrix<D> {
210318
/// is `<= tol` (non-positive or too small). This treats PSD degeneracy (and indefinite inputs)
211319
/// as singular/degenerate.
212320
/// Returns [`LaError::NonFinite`] if NaN/∞ is detected during factorization.
321+
///
322+
/// Note that an *asymmetric* input is **not** reported as an error in release builds —
323+
/// see the [Preconditions](#preconditions) section above.
213324
#[inline]
214325
pub fn ldlt(self, tol: f64) -> Result<Ldlt<D>, LaError> {
215326
Ldlt::factor(self, tol)
@@ -819,4 +930,109 @@ mod tests {
819930
gen_inf_norm_nonfinite_tests!(3);
820931
gen_inf_norm_nonfinite_tests!(4);
821932
gen_inf_norm_nonfinite_tests!(5);
933+
934+
// === is_symmetric / first_asymmetry (public LDLT preconditions helpers) ===
935+
936+
macro_rules! gen_is_symmetric_tests {
937+
($d:literal) => {
938+
paste! {
939+
#[test]
940+
fn [<is_symmetric_true_for_identity_ $d d>]() {
941+
let m = Matrix::<$d>::identity();
942+
assert!(m.is_symmetric(1e-12));
943+
assert_eq!(m.first_asymmetry(1e-12), None);
944+
}
945+
946+
#[test]
947+
fn [<is_symmetric_true_for_zero_ $d d>]() {
948+
let m = Matrix::<$d>::zero();
949+
assert!(m.is_symmetric(1e-12));
950+
assert_eq!(m.first_asymmetry(1e-12), None);
951+
}
952+
953+
#[test]
954+
fn [<is_symmetric_true_for_constructed_symmetric_ $d d>]() {
955+
// Construct A = M + Mᵀ so A is provably symmetric.
956+
let mut m = [[0.0f64; $d]; $d];
957+
for r in 0..$d {
958+
for c in 0..$d {
959+
#[allow(clippy::cast_precision_loss)]
960+
{
961+
m[r][c] = (r * $d + c) as f64;
962+
}
963+
}
964+
}
965+
let mut sym = [[0.0f64; $d]; $d];
966+
for r in 0..$d {
967+
for c in 0..$d {
968+
sym[r][c] = m[r][c] + m[c][r];
969+
}
970+
}
971+
let a = Matrix::<$d>::from_rows(sym);
972+
assert!(a.is_symmetric(1e-12));
973+
assert_eq!(a.first_asymmetry(1e-12), None);
974+
}
975+
976+
#[test]
977+
fn [<is_symmetric_false_for_asymmetric_offdiagonal_ $d d>]() {
978+
// Perturb a single off-diagonal entry so symmetry fails.
979+
let mut rows = [[0.0f64; $d]; $d];
980+
for i in 0..$d {
981+
rows[i][i] = 1.0;
982+
}
983+
rows[0][$d - 1] = 1.0;
984+
rows[$d - 1][0] = -1.0; // breaks symmetry
985+
let a = Matrix::<$d>::from_rows(rows);
986+
assert!(!a.is_symmetric(1e-12));
987+
assert_eq!(a.first_asymmetry(1e-12), Some((0, $d - 1)));
988+
}
989+
990+
#[test]
991+
fn [<is_symmetric_false_for_nan_offdiagonal_ $d d>]() {
992+
// A NaN off-diagonal must be detected as asymmetric.
993+
let mut rows = [[0.0f64; $d]; $d];
994+
for i in 0..$d {
995+
rows[i][i] = 1.0;
996+
}
997+
rows[0][1] = f64::NAN;
998+
rows[1][0] = f64::NAN;
999+
let a = Matrix::<$d>::from_rows(rows);
1000+
assert!(!a.is_symmetric(1e-12));
1001+
// (0, 1) is the first upper-triangular pair involving the NaN.
1002+
assert_eq!(a.first_asymmetry(1e-12), Some((0, 1)));
1003+
}
1004+
}
1005+
};
1006+
}
1007+
1008+
gen_is_symmetric_tests!(2);
1009+
gen_is_symmetric_tests!(3);
1010+
gen_is_symmetric_tests!(4);
1011+
gen_is_symmetric_tests!(5);
1012+
1013+
#[test]
1014+
fn is_symmetric_tolerance_scales_with_inf_norm() {
1015+
// Off-diagonal entries differ by 1e-6. With inf_norm ≈ 2e6, the
1016+
// relative tolerance 1e-12 yields eps ≈ 2e-6, which accepts the gap;
1017+
// a stricter tol of 1e-15 rejects it.
1018+
let a = Matrix::<2>::from_rows([[1.0e6, 1.0e6 + 1.0e-6], [1.0e6, 1.0e6]]);
1019+
assert!(a.is_symmetric(1e-12));
1020+
assert!(!a.is_symmetric(1e-15));
1021+
}
1022+
1023+
#[test]
1024+
fn first_asymmetry_returns_lexicographically_first_pair() {
1025+
// Two asymmetric pairs: (0, 2) and (1, 2). We must get (0, 2) first.
1026+
let a = Matrix::<3>::from_rows([[1.0, 0.0, 2.0], [0.0, 1.0, 3.0], [-2.0, -3.0, 1.0]]);
1027+
assert_eq!(a.first_asymmetry(1e-12), Some((0, 2)));
1028+
}
1029+
1030+
#[cfg(debug_assertions)]
1031+
#[test]
1032+
#[should_panic(expected = "rel_tol must be non-negative")]
1033+
fn first_asymmetry_debug_panics_on_negative_tol() {
1034+
// Mirrors the `debug_assert!(tol >= 0.0)` convention used by
1035+
// `Matrix::lu` / `Matrix::ldlt`.
1036+
let _ = Matrix::<2>::identity().first_asymmetry(-1.0);
1037+
}
8221038
}

0 commit comments

Comments
 (0)