feat!(api): enforce fallible numeric invariants#127
Merged
Conversation
- Introduce validated tolerance values for factorization and symmetry APIs. - Return typed errors for non-finite matrix, vector, determinant, and norm results. - Make determinant and vector operations propagate overflow/non-finite failures. - Update examples, docs, benchmarks, and property tests for the fallible API. - Document the v0.4.2 roadmap order for finite Matrix/Vector proof-type work. BREAKING CHANGE: tolerance arguments now use Tolerance instead of raw f64, Matrix::set returns Option<()>, determinant helpers return Result, Lu::det and Ldlt::det return Result<f64, LaError>, and Vector::dot / Vector::norm2_sq return Result<f64, LaError>. Closes #83
Contributor
📝 WalkthroughWalkthroughThis PR introduces a validated ChangesTolerance Type and Core Constants
Matrix Core Methods: Bounds, Norm, and Determinants
Vector Fallibility: Dot and Norm2_sq
LU and LDLT Factorization
Exact Arithmetic: Sign and Determinant
Documentation, Examples, and Benchmarks
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
- Coverage 99.55% 99.45% -0.11%
==========================================
Files 5 5
Lines 2255 2383 +128
==========================================
+ Hits 2245 2370 +125
- Misses 10 13 +3
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/matrix.rs (1)
355-374:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail when the scaled symmetry threshold overflows.
row_eps = rel_tol.mul_add(entry.abs(), row_eps)can still overflow for large finite matrices or loose tolerances. Once that happens,epsbecomes∞, andfirst_asymmetry()can returnOk(None)for a genuinely asymmetric matrix becausediff > epsis never true. ReturnLaError::NonFiniteas soon as the accumulator stops being finite.Suggested fix
fn symmetry_epsilon(&self, rel_tol: Tolerance) -> Result<f64, LaError> { let rel_tol = rel_tol.get(); let mut eps = rel_tol; for r in 0..D { let mut row_eps = 0.0; for c in 0..D { let entry = self.rows[r][c]; if !entry.is_finite() { cold_path(); return Err(LaError::non_finite_cell(r, c)); } row_eps = rel_tol.mul_add(entry.abs(), row_eps); + if !row_eps.is_finite() { + cold_path(); + return Err(LaError::non_finite_at(r)); + } } if row_eps > eps { eps = row_eps; } } Ok(eps) }As per coding guidelines, non-finite values (NaN, ±∞) must surface as
LaError::NonFinite { row, col }with source-location metadata.🤖 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/matrix.rs` around lines 355 - 374, In symmetry_epsilon, the accumulator row_eps (and consequently eps) can overflow to non-finite when using rel_tol.mul_add(entry.abs(), row_eps); after each mul_add check whether the new row_eps is finite and if not return Err(LaError::non_finite_cell(r, c)); likewise ensure eps is only updated with finite row_eps (and if eps becomes non-finite return LaError::non_finite_cell(r, c)); reference the symmetry_epsilon function, the row_eps and eps variables, and LaError::non_finite_cell to locate where to add these finiteness checks and early returns so non-finite accumulators surface as LaError::NonFinite with the proper row/col metadata.src/vector.rs (1)
72-124: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the rounding contract of
dot()andnorm2_sq().Both APIs now accumulate in
f64, but their docs do not state an absolute error bound or that no bound is provided. Please spell that out explicitly, the same way the determinant helpers do.As per coding guidelines, any
f64operation that accumulates rounding error must document its absolute bound or explicitly state that no bound is provided.🤖 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/vector.rs` around lines 72 - 124, Update the docs for Vector::dot and Vector::norm2_sq to state the rounding/accuracy contract: mention that both methods accumulate in f64 (using f64 mul_add on each term) and either provide the same absolute error bound used by the determinant helpers or explicitly state that no absolute bound is provided; mirror the phrasing and examples from the determinant helper doc comments, include that intermediate rounding occurs and the returned Result is checked for non-finite accumulation, and reference the functions by name (dot, norm2_sq) so readers can find the implementation details.examples/exact_det_3x3.rs (1)
12-29: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winInconsistent error handling pattern across examples.
This example uses
fn main()without aResultreturn and chains.unwrap().unwrap()to extract the determinant (line 27), which is not idiomatic. Other examples in this PR layer (e.g.,det_5x5.rs, README snippets) consistently usefn main() -> Result<(), LaError>with?for error propagation.Recommend updating to match the established pattern:
♻️ Refactor to use Result and ? operator
-fn main() { +fn main() -> Result<(), LaError> { // Base matrix: rows in arithmetic progression → exactly singular (det = 0). // [[1, 2, 3], // [4, 5, 6], // [7, 8, 9]] // // Perturb entry (0,0) by 2^-50 ≈ 8.9e-16. // Exact det = 2^-50 × cofactor(0,0) = 2^-50 × (5×9 − 6×8) = −3 × 2^-50. let perturbation = f64::from_bits(0x3CD0_0000_0000_0000); // 2^-50 let m = Matrix::<3>::from_rows([ [1.0 + perturbation, 2.0, 3.0], [4.0, 5.0, 6.0], [7.0, 8.0, 9.0], ]); - let det_f64_approx = m.det_direct().unwrap().unwrap(); - let det_exact = m.det_exact().unwrap(); - let det_exact_as_f64 = m.det_exact_f64().unwrap(); + let det_f64_approx = m.det_direct()?.expect("D=3 is supported by det_direct"); + let det_exact = m.det_exact()?; + let det_exact_as_f64 = m.det_exact_f64()?; println!("Near-singular 3×3 matrix (perturbation = 2^-50 ≈ {perturbation:.2e}):"); for r in 0..3 { print!(" ["); for c in 0..3 { if c > 0 { print!(", "); } - print!("{:22.18}", m.get(r, c).unwrap()); + print!("{:22.18}", m.get(r, c)?); } println!("]"); } println!(); println!("f64 det_direct() = {det_f64_approx:+.6e}"); println!("det_exact() = {det_exact}"); println!("det_exact_f64() = {det_exact_as_f64:+.6e}"); println!(); println!("The exact determinant is −3/2^50 ≈ −2.66e-15."); + + Ok(()) }🤖 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 `@examples/exact_det_3x3.rs` around lines 12 - 29, The main function in this example uses panicking unwraps (fn main()) and double .unwrap().unwrap() on det_direct; change it to the established pattern by making fn main() -> Result<(), LaError> and propagate errors with ? on calls to Matrix::<3>::det_direct(), det_exact(), and det_exact_f64() (e.g. replace the unwrap chain with det_direct()? and det_exact()? etc.), then end with Ok(()) so the example matches det_5x5.rs and README snippets; keep the same Matrix::<3> instantiation and variable names (det_f64_approx, det_exact, det_exact_as_f64) but obtain their values via ? instead of unwrap.
🧹 Nitpick comments (2)
tests/proptest_vector.rs (1)
43-55: ⚡ Quick winAdd adversarial generators for the new vector error contract.
This property test still samples only
small_f64(), sodot()andnorm2_sq()can never hit the newErr(LaError::NonFinite { .. })paths. Add a second case with large or injected non-finite components and assert the structured error.As per coding guidelines, property-based tests under
tests/proptest_*.rsmust include adversarial inputs alongside well-conditioned inputs.🤖 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 `@tests/proptest_vector.rs` around lines 43 - 55, The property test currently only uses small_f64() so calls to Vector::<$d>::dot and norm2_sq never exercise the Err(LaError::NonFinite { .. }) path; update the test to include a second case that generates adversarial vectors (e.g., large_f64() and injected non-finite values like NaN/Inf) alongside the existing well-conditioned generator, then for those adversarial inputs assert that a.dot(b) and a.dot(a)/a.norm2_sq() return Err(LaError::NonFinite { .. }) and match the structured error rather than using assert_abs_diff_eq!, locating changes around the usages of dot, norm2_sq, and the vector generator setup.tests/proptest_matrix.rs (1)
127-128: ⚡ Quick winProperty-test the new matrix error paths instead of only
unwrap()ing them.
small_f64()and the SPD generator only produce well-conditioned finite matrices, so the new fallible branches exercised byinf_norm()andldlt.det()never run here. Please add at least one adversarial generator with NaN / large-magnitude / asymmetric inputs and assert the structuredLaErrorresults.As per coding guidelines, property-based tests under
tests/proptest_*.rsmust include adversarial inputs alongside well-conditioned inputs.Also applies to: 214-214
🤖 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 `@tests/proptest_matrix.rs` around lines 127 - 128, Add an adversarial generator alongside small_f64() / the SPD generator that can produce NaN, huge-magnitude, and asymmetric matrices and update the assertions for inf_norm() and ldlt.det() to check for the structured LaError rather than unwrapping; specifically, in the property test that calls Matrix::inf_norm() and LDLT::det() replace unwrap() usage with pattern matching or prop_assert matches to assert the returned Result is Err(LaError::...) for the adversarial cases and still assert Ok(expected) for well-conditioned cases, referencing the inf_norm(), ldlt.det(), LaError, small_f64(), and the SPD generator to locate the test logic to modify.
🤖 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.
Inline comments:
In `@examples/exact_solve_3x3.rs`:
- Around line 29-34: The example currently uses unwrap() on fallible operations
(a.lu(Tolerance::new(0.0).unwrap()).unwrap().solve_vec(b).unwrap().into_array())
which panics; change main to return Result<(), LaError> and propagate errors
with ? instead of unwrap so each fallible call (Tolerance::new, Matrix::lu,
LU::solve_vec) uses ? and the final result is handled; update the function
signature to fn main() -> Result<(), LaError> and replace the chained unwraps
with the same calls using ? to return errors up the stack.
In `@src/matrix.rs`:
- Around line 192-221: Update the inf_norm() documentation to state the
rounding/accuracy contract: describe that row sums are accumulated in f64 and
either give a specific absolute rounding bound for the returned value or
explicitly state that no rounding bound is provided (i.e., result is heuristic
and may incur typical floating‑point accumulation error). Place this sentence
near the existing Non-finite handling / # Errors section of the inf_norm() doc
comment so callers of inf_norm() know whether the norm is certified or has no
guaranteed rounding bound.
---
Outside diff comments:
In `@examples/exact_det_3x3.rs`:
- Around line 12-29: The main function in this example uses panicking unwraps
(fn main()) and double .unwrap().unwrap() on det_direct; change it to the
established pattern by making fn main() -> Result<(), LaError> and propagate
errors with ? on calls to Matrix::<3>::det_direct(), det_exact(), and
det_exact_f64() (e.g. replace the unwrap chain with det_direct()? and
det_exact()? etc.), then end with Ok(()) so the example matches det_5x5.rs and
README snippets; keep the same Matrix::<3> instantiation and variable names
(det_f64_approx, det_exact, det_exact_as_f64) but obtain their values via ?
instead of unwrap.
In `@src/matrix.rs`:
- Around line 355-374: In symmetry_epsilon, the accumulator row_eps (and
consequently eps) can overflow to non-finite when using
rel_tol.mul_add(entry.abs(), row_eps); after each mul_add check whether the new
row_eps is finite and if not return Err(LaError::non_finite_cell(r, c));
likewise ensure eps is only updated with finite row_eps (and if eps becomes
non-finite return LaError::non_finite_cell(r, c)); reference the
symmetry_epsilon function, the row_eps and eps variables, and
LaError::non_finite_cell to locate where to add these finiteness checks and
early returns so non-finite accumulators surface as LaError::NonFinite with the
proper row/col metadata.
In `@src/vector.rs`:
- Around line 72-124: Update the docs for Vector::dot and Vector::norm2_sq to
state the rounding/accuracy contract: mention that both methods accumulate in
f64 (using f64 mul_add on each term) and either provide the same absolute error
bound used by the determinant helpers or explicitly state that no absolute bound
is provided; mirror the phrasing and examples from the determinant helper doc
comments, include that intermediate rounding occurs and the returned Result is
checked for non-finite accumulation, and reference the functions by name (dot,
norm2_sq) so readers can find the implementation details.
---
Nitpick comments:
In `@tests/proptest_matrix.rs`:
- Around line 127-128: Add an adversarial generator alongside small_f64() / the
SPD generator that can produce NaN, huge-magnitude, and asymmetric matrices and
update the assertions for inf_norm() and ldlt.det() to check for the structured
LaError rather than unwrapping; specifically, in the property test that calls
Matrix::inf_norm() and LDLT::det() replace unwrap() usage with pattern matching
or prop_assert matches to assert the returned Result is Err(LaError::...) for
the adversarial cases and still assert Ok(expected) for well-conditioned cases,
referencing the inf_norm(), ldlt.det(), LaError, small_f64(), and the SPD
generator to locate the test logic to modify.
In `@tests/proptest_vector.rs`:
- Around line 43-55: The property test currently only uses small_f64() so calls
to Vector::<$d>::dot and norm2_sq never exercise the Err(LaError::NonFinite { ..
}) path; update the test to include a second case that generates adversarial
vectors (e.g., large_f64() and injected non-finite values like NaN/Inf)
alongside the existing well-conditioned generator, then for those adversarial
inputs assert that a.dot(b) and a.dot(a)/a.norm2_sq() return
Err(LaError::NonFinite { .. }) and match the structured error rather than using
assert_abs_diff_eq!, locating changes around the usages of dot, norm2_sq, and
the vector generator setup.
🪄 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: 15349511-692b-42de-b29c-f34a71cb1f8e
📒 Files selected for processing (23)
AGENTS.mdCHANGELOG.mdCargo.tomlREADME.mdbenches/exact.rsbenches/vs_linalg.rsdocs/roadmap.mdexamples/const_det_4x4.rsexamples/det_5x5.rsexamples/exact_det_3x3.rsexamples/exact_solve_3x3.rsexamples/ldlt_solve_3x3.rsjustfilesrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/vector.rstests/proptest_exact.rstests/proptest_factorizations.rstests/proptest_matrix.rstests/proptest_vector.rs
Comment on lines
+29
to
+34
| let lu_x = a | ||
| .lu(Tolerance::new(0.0).unwrap()) | ||
| .unwrap() | ||
| .solve_vec(b) | ||
| .unwrap() | ||
| .into_array(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Propagate fallible calls in the example instead of panicking.
Line [30]-Line [33] currently unwrap fallible operations; this example should model the new fallible API and return Result<(), LaError> from main with ?.
Suggested change
-fn main() {
+fn main() -> Result<(), LaError> {
@@
- let lu_x = a
- .lu(Tolerance::new(0.0).unwrap())
- .unwrap()
- .solve_vec(b)
- .unwrap()
- .into_array();
+ let lu_x = a.lu(Tolerance::new(0.0)?)?.solve_vec(b)?.into_array();
@@
- let exact_x = a.solve_exact(b).unwrap();
- let exact_x_f64 = a.solve_exact_f64(b).unwrap().into_array();
+ let exact_x = a.solve_exact(b)?;
+ let exact_x_f64 = a.solve_exact_f64(b)?.into_array();
@@
+ Ok(())
}As per coding guidelines: **/*.rs: “Use Result<_, LaError> for all fallible operations. Panics are reserved for debug-only precondition violations (e.g., LDLT symmetry check) and must be documented on the method.”
🤖 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 `@examples/exact_solve_3x3.rs` around lines 29 - 34, The example currently uses
unwrap() on fallible operations
(a.lu(Tolerance::new(0.0).unwrap()).unwrap().solve_vec(b).unwrap().into_array())
which panics; change main to return Result<(), LaError> and propagate errors
with ? instead of unwrap so each fallible call (Tolerance::new, Matrix::lu,
LU::solve_vec) uses ? and the final result is handled; update the function
signature to fn main() -> Result<(), LaError> and replace the chained unwraps
with the same calls using ? to return errors up the stack.
Comment on lines
192
to
+221
| /// Infinity norm (maximum absolute row sum). | ||
| /// | ||
| /// # Non-finite handling | ||
| /// If any entry is NaN, the result is NaN. NaN is detected explicitly | ||
| /// because a naive `row_sum > max_row_sum` comparison silently skips NaN | ||
| /// rows (every ordered comparison against NaN is `false`). If any entry | ||
| /// is infinite (and no entry is NaN), the result is `+∞`. | ||
| /// Non-finite entries are rejected with source coordinates instead of | ||
| /// silently propagating NaN or infinity through the norm. | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// use la_stack::prelude::*; | ||
| /// | ||
| /// # fn main() -> Result<(), LaError> { | ||
| /// let m = Matrix::<2>::from_rows([[1.0, -2.0], [3.0, 4.0]]); | ||
| /// assert!((m.inf_norm() - 7.0).abs() <= 1e-12); | ||
| /// assert!((m.inf_norm()? - 7.0).abs() <= 1e-12); | ||
| /// | ||
| /// // NaN entries propagate to the norm. | ||
| /// // NaN entries are rejected with coordinates. | ||
| /// let nan = Matrix::<2>::from_rows([[f64::NAN, 1.0], [2.0, 3.0]]); | ||
| /// assert!(nan.inf_norm().is_nan()); | ||
| /// assert_eq!( | ||
| /// nan.inf_norm(), | ||
| /// Err(LaError::NonFinite { | ||
| /// row: Some(0), | ||
| /// col: 0, | ||
| /// }) | ||
| /// ); | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| /// | ||
| /// # Errors | ||
| /// Returns [`LaError::NonFinite`] when any entry is NaN or infinity, or when | ||
| /// a row sum overflows to NaN or infinity. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the rounding contract of inf_norm().
This method now accumulates f64 row sums, but the doc comment neither gives an absolute bound nor states that no bound is provided. Please add that sentence explicitly so callers know whether this result is certified or heuristic.
As per coding guidelines, any f64 operation that accumulates rounding error must document its absolute bound or explicitly state that no bound is provided.
🤖 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/matrix.rs` around lines 192 - 221, Update the inf_norm() documentation to
state the rounding/accuracy contract: describe that row sums are accumulated in
f64 and either give a specific absolute rounding bound for the returned value or
explicitly state that no rounding bound is provided (i.e., result is heuristic
and may incur typical floating‑point accumulation error). Place this sentence
near the existing Non-finite handling / # Errors section of the inf_norm() doc
comment so callers of inf_norm() know whether the norm is certified or has no
guaranteed rounding bound.
This was referenced Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: tolerance arguments now use Tolerance instead of raw f64, Matrix::set returns Option<()>, determinant helpers return Result, Lu::det and Ldlt::det return Result<f64, LaError>, and Vector::dot / Vector::norm2_sq return Result<f64, LaError>.
Closes #83
Summary by CodeRabbit
New Features
Tolerancetype for robust tolerance handling across factorization and symmetry operations.Bug Fixes
Resulttypes instead of panicking on invalid operations or non-finite values.Documentation
Chores