feat!(matrix): enforce fallible matrix invariants#124
Conversation
- Add typed errors for unsupported runtime dimensions, out-of-bounds indices, invalid tolerances, and asymmetric matrices. - Add checked matrix accessors and stack-matrix runtime dispatch up to D=7. - Validate symmetry, tolerances, and non-finite entries before symmetry checks and factorization return typed errors. - Update LDLT documentation and README guidance for runtime asymmetry rejection. BREAKING CHANGE: Matrix::is_symmetric now returns Result<bool, LaError>, and Matrix::first_asymmetry now returns Result<Option<(usize, usize)>, LaError>. BREAKING CHANGE: LaError no longer implements Eq because InvalidTolerance stores the original f64 tolerance value. BREAKING CHANGE: Matrix::ldlt rejects asymmetric inputs with LaError::Asymmetric, and LU/LDLT reject negative, NaN, or infinite tolerances with LaError::InvalidTolerance.
📝 WalkthroughWalkthroughThe PR extends the linear-algebra error surface with four new ChangesError Type and Validation API Surface
Matrix Access and Symmetry Validation
LDLT Factorization Precondition Enforcement
LU and Documentation Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #124 +/- ##
==========================================
- Coverage 99.62% 99.55% -0.08%
==========================================
Files 5 5
Lines 2150 2255 +105
==========================================
+ Hits 2142 2245 +103
- Misses 8 10 +2
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib.rs (1)
710-714: 💤 Low valueConsider adding D=6 to the dispatch test invocations.
The
gen_stack_matrix_dispatch_tests!macro is invoked for D=2,3,4,5,7 but skips D=6. While the macro arm for dimension 6 exists intry_with_stack_matrix!, addinggen_stack_matrix_dispatch_tests!(6);would ensure full coverage of all supported dispatch arms.🧪 Proposed fix to add D=6 test
gen_stack_matrix_dispatch_tests!(2); gen_stack_matrix_dispatch_tests!(3); gen_stack_matrix_dispatch_tests!(4); gen_stack_matrix_dispatch_tests!(5); + gen_stack_matrix_dispatch_tests!(6); gen_stack_matrix_dispatch_tests!(7);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 710 - 714, Add the missing D=6 invocation to the dispatch test generators: call gen_stack_matrix_dispatch_tests!(6); alongside the existing calls (gen_stack_matrix_dispatch_tests!(2/3/4/5/7)) so the dispatch coverage matches the try_with_stack_matrix! arm for dimension 6; this ensures gen_stack_matrix_dispatch_tests! covers D=6 as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib.rs`:
- Around line 710-714: Add the missing D=6 invocation to the dispatch test
generators: call gen_stack_matrix_dispatch_tests!(6); alongside the existing
calls (gen_stack_matrix_dispatch_tests!(2/3/4/5/7)) so the dispatch coverage
matches the try_with_stack_matrix! arm for dimension 6; this ensures
gen_stack_matrix_dispatch_tests! covers D=6 as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 196d09af-757f-4c41-8cd3-b9b7fc93805c
⛔ Files ignored due to path filters (1)
docs/assets/la-stack.jpgis excluded by!**/*.jpg
📒 Files selected for processing (7)
Cargo.tomlREADME.mdsrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rstests/proptest_matrix.rs
BREAKING CHANGE: Matrix::is_symmetric now returns Result<bool, LaError>, and Matrix::first_asymmetry now returns Result<Option<(usize, usize)>, LaError>.
BREAKING CHANGE: LaError no longer implements Eq because InvalidTolerance stores the original f64 tolerance value.
BREAKING CHANGE: Matrix::ldlt rejects asymmetric inputs with LaError::Asymmetric, and LU/LDLT reject negative, NaN, or infinite tolerances with LaError::InvalidTolerance.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation