docs: strengthen LDLT symmetry precondition and add is_symmetric API (#84)#93
Conversation
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>
acgetchell
added
documentation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR addresses issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
+ Coverage 88.65% 90.96% +2.30%
==========================================
Files 5 5
Lines 485 498 +13
==========================================
+ Hits 430 453 +23
+ Misses 55 45 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add unit tests to exercise internal safety nets in the LU and LDLT diagonal solve routines. These tests manually construct factors with invalid diagonals (NaN or sub-tolerance) to verify that solve_vec correctly surfaces NonFinite and Singular errors, even though these states are unreachable via the standard factorization APIs. Refs: #84
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ldlt.rs (1)
220-227: Avoid constant-falsedebug_assert!to prevent linter warnings.The code currently uses
debug_assert!(false, ...)which strict Clippy configurations flag as a constant assertion. Extract the asymmetry check into a variable and use its negation to make the assertion condition non-constant while preserving the same debug-only panic behavior:♻️ Proposed refactor
+ let asymmetry = a.first_asymmetry(1e-12); + if let Some((r, c)) = asymmetry { - if let Some((r, c)) = a.first_asymmetry(1e-12) { let diff = (a.rows[r][c] - a.rows[c][r]).abs(); let eps = 1e-12 * a.inf_norm().max(1.0); debug_assert!( - false, + asymmetry.is_none(), "matrix must be symmetric (diff={diff}, eps={eps}) at ({r}, {c}); \ pre-validate with Matrix::is_symmetric before calling ldlt" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ldlt.rs` around lines 220 - 227, The debug_assert! currently uses a constant false which triggers linter warnings; change it to assert the non-constant asymmetry condition instead by computing a boolean like let asymmetric = diff > eps (using the existing diff and eps calculations from the first_asymmetry branch) and then call debug_assert!(asymmetric, "matrix must be symmetric (diff={diff}, eps={eps}) at ({r}, {c}); pre-validate with Matrix::is_symmetric before calling ldlt") or, better, use debug_assert!(!asymmetric, ...) depending on the intended panic on symmetry violation—update the assertion in the block that handles a.first_asymmetry so the condition references that computed boolean (and preserve the same message and use of r, c, diff, eps).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/matrix.rs`:
- Around line 227-236: The asymmetry check in first_asymmetry uses eps = rel_tol
* self.inf_norm().max(1.0), which becomes +∞ when inf_norm() is infinite and
then masks infinite diffs; update first_asymmetry to handle infinite norms by
either (a) capping/using a finite fallback for eps when eps.is_infinite() (e.g.
use rel_tol or rel_tol * 1.0) or (b) explicitly treat diff.is_infinite() as an
asymmetry regardless of eps; also add the suggested regression test that
constructs Matrix::<2> with an ∞ off-diagonal and asserts first_asymmetry(1e-12)
returns Some((0,1)) and is_symmetric(1e-12) is false to prevent regressions.
---
Nitpick comments:
In `@src/ldlt.rs`:
- Around line 220-227: The debug_assert! currently uses a constant false which
triggers linter warnings; change it to assert the non-constant asymmetry
condition instead by computing a boolean like let asymmetric = diff > eps (using
the existing diff and eps calculations from the first_asymmetry branch) and then
call debug_assert!(asymmetric, "matrix must be symmetric (diff={diff},
eps={eps}) at ({r}, {c}); pre-validate with Matrix::is_symmetric before calling
ldlt") or, better, use debug_assert!(!asymmetric, ...) depending on the intended
panic on symmetry violation—update the assertion in the block that handles
a.first_asymmetry so the condition references that computed boolean (and
preserve the same message and use of r, c, diff, eps).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b247411b-c6c2-4ee7-99f7-8e3749983ed8
📒 Files selected for processing (4)
README.mdsrc/ldlt.rssrc/lu.rssrc/matrix.rs
Update Matrix::first_asymmetry to flag any non-finite difference between off-diagonal pairs as an asymmetry. This prevents cases where a single infinite entry paired with a finite entry would incorrectly pass as symmetric because the matrix's infinite norm blew the tolerance up to infinity, making the comparison `diff > eps` return false. Refs: #84
1 participant
Closes #84.
Summary by CodeRabbit
New Features
Documentation
Tests