fix(invariants): revalidate public compute inputs#135
Conversation
- Parse Matrix and Vector storage into private finite proof-bearing types at public compute boundaries. - Reject unchecked non-finite storage before LU, LDLT, determinant, norm, dot, and exact-arithmetic paths can proceed. - Keep unchecked proof-wrapper constructors crate-private for internal paths with local finiteness proofs. - Document the private proof-bearing invariant model in the API overview and roadmap.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
+ Coverage 99.29% 99.33% +0.03%
==========================================
Files 5 5
Lines 2707 2844 +137
==========================================
+ Hits 2688 2825 +137
Misses 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR refactors the internal finite-invariant boundary by introducing validated (checked) and unchecked constructor paths for ChangesFinite-Proof Boundary Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR involves coordinated changes across multiple computation layers (matrix, vector, factorization, exact arithmetic, and test infrastructure) with a consistent but non-trivial architectural pattern: splitting checked and unchecked constructor paths, then systematically revalidating at public boundaries. Understanding the pattern across one or two layers transfers well, but verifying consistent application throughout the codebase and ensuring no false-positive rejections is logic-dense work. The heterogeneous mix of constructors, boundary validations, and test macro coverage increases review complexity. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/exact.rs (1)
846-847: ⚡ Quick winExercise the unchecked internal wrapper path here.
This macro is supposed to cover the crate-internal finite path, but Lines 846-847 re-enter the validating constructors instead. Switching these fixtures to
new_unchecked(...)would keep this test focused on the internal proof-bearing contract introduced by this PR.♻️ Proposed fixture change
- let a = FiniteMatrix::<$d>::new(Matrix::<$d>::identity()).unwrap(); - let b = FiniteVector::<$d>::new(Vector::<$d>::new([1.0; $d])).unwrap(); + let a = FiniteMatrix::<$d>::new_unchecked(Matrix::<$d>::identity()); + let b = FiniteVector::<$d>::new_unchecked( + Vector::<$d>::new_unchecked([1.0; $d]), + );🤖 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/exact.rs` around lines 846 - 847, The test currently constructs FiniteMatrix::<$d> and FiniteVector::<$d> via the validating constructors FiniteMatrix::new(...) and FiniteVector::new(...); change those calls to the internal unchecked constructors FiniteMatrix::new_unchecked(...) and FiniteVector::new_unchecked(...) so the fixtures use the crate-internal unchecked wrapper path (wrap Matrix::<$d>::identity() and Vector::<$d>::new([1.0; $d]) with new_unchecked instead of new).
🤖 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 `@CHANGELOG.md`:
- Around line 45-48: The CHANGELOG.md was edited manually (the "Simplify finite
proof wrappers" entry / commit 54b603c appears hand-modified); revert any manual
edits to CHANGELOG.md (restore from the branch/main or last-generated version)
and regenerate the changelog using the project tooling—run just changelog (or
just changelog-unreleased <version> if prepping a release) to recreate the file,
verify the generated entry for "Simplify finite proof wrappers" is present and
accurate, then commit the regenerated CHANGELOG.md.
In `@src/ldlt.rs`:
- Around line 190-194: The doc comment for solve_vec currently lists error cases
but omits any statement about rounding error bounds; update the doc for the
solve_vec method to explicitly state that it performs floating-point
substitution and does not provide a certified absolute/rounding error bound
(unless you intend to provide one), e.g. add a sentence like “This method
performs floating-point substitution and does not guarantee a certified absolute
rounding-error bound.” Keep the existing error notes (e.g. LaError::NonFinite)
and follow the same phrasing used in other modules (matrix/lu/ldlt) for
consistency.
In `@src/lu.rs`:
- Around line 142-146: Update the doc comment for the solve_vec method to
explicitly state that no certified rounding/absolute error bound is provided for
the floating-point forward/back substitution it performs; reference the function
name solve_vec and mention that callers should not expect an absolute error
bound (unlike det_errbound/ERR_COEFF_*), and keep existing clarification about
failure returning LaError::NonFinite and the revalidation via Vector::try_new.
In `@src/matrix.rs`:
- Around line 670-671: Update the inf_norm documentation to remove the outdated
claim that non-finite values are rejected before storage and instead state that
stored NaN/∞ may be encountered due to the crate's unchecked-storage model;
explicitly mention that inf_norm will return LaError::NonFinite when it
encounters stored NaN/∞ or when a row sum overflows to non-finite, and keep the
existing # Errors section consistent with this behavior (refer to inf_norm and
LaError::NonFinite in the text).
---
Nitpick comments:
In `@src/exact.rs`:
- Around line 846-847: The test currently constructs FiniteMatrix::<$d> and
FiniteVector::<$d> via the validating constructors FiniteMatrix::new(...) and
FiniteVector::new(...); change those calls to the internal unchecked
constructors FiniteMatrix::new_unchecked(...) and
FiniteVector::new_unchecked(...) so the fixtures use the crate-internal
unchecked wrapper path (wrap Matrix::<$d>::identity() and
Vector::<$d>::new([1.0; $d]) with new_unchecked instead of new).
🪄 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: 51045dbd-5a49-44df-8e06-83c5c9f64e47
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mddocs/roadmap.mdsrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/vector.rs
| /// Returns [`LaError::NonFinite`] if the right-hand side contains | ||
| /// NaN/infinity or a computed substitution intermediate overflows to NaN or | ||
| /// infinity. The right-hand side is revalidated at this boundary before | ||
| /// entering substitution; public callers normally encounter the same | ||
| /// rejection earlier through [`Vector::try_new`](crate::Vector::try_new). |
There was a problem hiding this comment.
Document that solve_vec has no certified rounding bound.
This method performs floating-point substitution, but the docs still only describe error cases. Please add the usual note that no certified absolute error bound is provided unless you want to document one explicitly. As per coding guidelines, src/{matrix,lu,ldlt}.rs: "Any f64 operation that can accumulate rounding error either documents its absolute bound (det_errbound, ERR_COEFF_*) or explicitly states 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/ldlt.rs` around lines 190 - 194, The doc comment for solve_vec currently
lists error cases but omits any statement about rounding error bounds; update
the doc for the solve_vec method to explicitly state that it performs
floating-point substitution and does not provide a certified absolute/rounding
error bound (unless you intend to provide one), e.g. add a sentence like “This
method performs floating-point substitution and does not guarantee a certified
absolute rounding-error bound.” Keep the existing error notes (e.g.
LaError::NonFinite) and follow the same phrasing used in other modules
(matrix/lu/ldlt) for consistency.
| /// Returns [`LaError::NonFinite`] if the right-hand side contains | ||
| /// NaN/infinity or a computed substitution intermediate overflows to NaN or | ||
| /// infinity. The right-hand side is revalidated at this boundary before | ||
| /// entering substitution; public callers normally encounter the same | ||
| /// rejection earlier through [`Vector::try_new`](crate::Vector::try_new). |
There was a problem hiding this comment.
Document that solve_vec has no certified rounding bound.
This path does floating-point forward/back substitution, but the docs still only describe failure cases. The repo guideline requires these methods to either state an absolute bound or explicitly say none is provided. As per coding guidelines, src/{matrix,lu,ldlt}.rs: "Any f64 operation that can accumulate rounding error either documents its absolute bound (det_errbound, ERR_COEFF_*) or explicitly states 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/lu.rs` around lines 142 - 146, Update the doc comment for the solve_vec
method to explicitly state that no certified rounding/absolute error bound is
provided for the floating-point forward/back substitution it performs; reference
the function name solve_vec and mention that callers should not expect an
absolute error bound (unlike det_errbound/ERR_COEFF_*), and keep existing
clarification about failure returning LaError::NonFinite and the revalidation
via Vector::try_new.
| /// Returns [`LaError::NonFinite`] when stored entries are NaN/infinity or a | ||
| /// row sum overflows to NaN or infinity. |
There was a problem hiding this comment.
Update the inf_norm docs to match the unchecked-storage model.
The # Errors section now correctly says stored NaN/∞ can reach this method, but the earlier “Non-finite handling” paragraph still says those values are rejected before storage. Please reconcile those two statements so the public docs reflect the new crate-internal unchecked path.
🤖 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 670 - 671, Update the inf_norm documentation to
remove the outdated claim that non-finite values are rejected before storage and
instead state that stored NaN/∞ may be encountered due to the crate's
unchecked-storage model; explicitly mention that inf_norm will return
LaError::NonFinite when it encounters stored NaN/∞ or when a row sum overflows
to non-finite, and keep the existing # Errors section consistent with this
behavior (refer to inf_norm and LaError::NonFinite in the text).
1 participant
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes