feat(exact)!: make exact f64 conversions strict#149
Merged
Conversation
- Add explicit rounded exact-to-f64 APIs for determinant and solve results - Report exact conversion failures with typed Unrepresentable reasons - Remove finite proof wrapper APIs now that Matrix and Vector carry finiteness directly - Move error and tolerance contracts into first-class modules with prelude exports - Update exact benchmarks to distinguish strict Result paths from rounded f64 paths - Document and exercise the rounded fallback pattern for RequiresRounding errors BREAKING CHANGE: det_exact_f64 and solve_exact_f64 now return LaError::Unrepresentable instead of rounding or reporting Overflow for exact values that cannot be represented exactly as finite f64. Call det_exact_rounded_f64 or solve_exact_rounded_f64 to opt into rounded finite f64 results. BREAKING CHANGE: FiniteMatrix, FiniteVector, and Matrix::from_rows are removed from the public API. Use Matrix, Vector, Matrix::try_from_rows, and Vector::try_new instead. Closes #148
Contributor
📝 WalkthroughWalkthroughEnforces strict exact→f64 conversion contracts with opt-in rounded fallbacks; removes FiniteMatrix/FiniteVector and centralizes finiteness validation on Matrix/Vector; introduces typed ChangesStrict exact-to-f64 conversion with finite-by-construction Matrix/Vector
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
- Coverage 99.58% 98.23% -1.36%
==========================================
Files 5 7 +2
Lines 2664 3228 +564
==========================================
+ Hits 2653 3171 +518
- Misses 11 57 +46
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:
|
![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: 4
🤖 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`:
- Line 34: Add explanatory sub-bullets under the "Feat!(api): make Matrix and
Vector finite by construction" entry describing the breaking contract changes:
state that Matrix and Vector are now finite by construction and validated at
construction boundaries, note that the previous public proof wrappers
FiniteMatrix and FiniteVector have been removed, and document that public
constructors are now fallible (e.g., try_from_rows, try_new) so callers must
handle construction errors; keep the format consistent with other entries and
include the commit hash reference.
- Around line 20-24: Update the changelog line to use the correct error
terminology: replace "preserving Overflow for finite-f64 conversion failures"
with wording that references LaError::Unrepresentable (or "preserving
Unrepresentable for finite-f64 conversion failures") to match the actual API
behavior of det_exact_f64 and solve_exact_f64 which return
LaError::Unrepresentable on exact-to-f64 conversion failures; ensure the phrase
explicitly mentions LaError::Unrepresentable so it aligns with src/error.rs and
the PR intent.
In `@src/matrix.rs`:
- Around line 653-686: The determinant calculation in the 2x2, 3x3, and 4x4
cases uses nested mul_add chains that always evaluate all minors and cofactors
even when their coefficients are zero. This causes 0.0 * ∞ = NaN when a
coefficient like self.rows[0][1] is zero but the corresponding minor contains
very large values. To fix this, add conditional checks in each branch (the 2
case, 3 case, and 4 case) to skip computing and multiplying minors/cofactors
when their corresponding coefficients are zero. For the 3x3 case, check if
self.rows[0][1] is zero before evaluating m01; for the 4x4 case, check the
corresponding r[0][j] coefficients before computing c01, c02, c03. Short-circuit
these branches to prevent NaN poisoning of the final determinant result.
In `@src/tolerance.rs`:
- Around line 93-99: The LDLT_SYMMETRY_REL_TOL constant in module tolerance is
declared with pub(crate) which is redundant and breaks CI; change its visibility
to pub so the crate-root re-export in src/lib.rs works correctly—locate the
constant named LDLT_SYMMETRY_REL_TOL (Tolerance::new_unchecked(1e-12)) in
tolerance.rs and replace pub(crate) with pub to resolve the warning.
🪄 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: e8fac178-e100-491d-adfa-9b8661444bd8
📒 Files selected for processing (23)
CHANGELOG.mdREADME.mdbenches/exact.rsdocs/BENCHMARKING.mddocs/archive/changelog/0.1.mddocs/archive/changelog/0.2.mddocs/archive/changelog/0.3.mdexamples/exact_solve_3x3.rsjustfilesemgrep.yamlsrc/error.rssrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/tolerance.rssrc/vector.rstests/proptest_exact.rstests/regressions.rstests/semgrep/src/project_rules/finite_api_contract.rstests/semgrep/src/project_rules/raw_f64_constructors.rstests/semgrep/src/project_rules/readme_doctest_mirrors.rs
💤 Files with no reviewable changes (4)
- tests/semgrep/src/project_rules/finite_api_contract.rs
- docs/archive/changelog/0.3.md
- docs/archive/changelog/0.2.md
- docs/archive/changelog/0.1.md
- Make Matrix and Vector the finite-by-construction public types for exact arithmetic. - Add rounded exact-to-f64 APIs for determinant and solve callers that want explicit lossy conversion. - Return typed Unrepresentable reasons when strict exact-to-f64 conversion would round or become non-finite. - Specialize D4 exact determinants and keep determinant/error-bound zero coefficients from evaluating overflowing absent terms. - Update exact benchmark comparison reporting to compare strict and rounded APIs against legacy v0.4.2 rows. BREAKING CHANGE: strict exact-to-f64 APIs now return LaError::Unrepresentable instead of silently rounding, public Matrix and Vector construction is fallible, and the previous finite proof wrapper APIs are removed.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CHANGELOG.md (1)
26-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the current error contract name in changelog entry.
Line 26 still says finite-f64 conversion failures preserve
Overflow, but the strict exact→f64 contract now usesLaError::Unrepresentable. This line should match the current API terminology.Suggested fix
- - Convert det_exact_f64 directly from the shared Bareiss integer/exponent pair while preserving Overflow for finite-f64 conversion failures + - Convert det_exact_f64 directly from the shared Bareiss integer/exponent pair while preserving LaError::Unrepresentable for finite-f64 conversion failures🤖 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 `@CHANGELOG.md` at line 26, Update the changelog sentence to use the current error name: replace the mention of preserving `Overflow` for finite-f64 conversion failures with `LaError::Unrepresentable` so it reflects the strict exact→f64 contract; locate the line referencing `det_exact_f64` and change the error term to `LaError::Unrepresentable`.
🤖 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 `@src/matrix.rs`:
- Around line 350-363: The inf-norm accumulation currently waits until the end
of the row and calls LaError::non_finite_at(r), losing the column index; change
the inner loop in the method (the loops using variables r, c and row_sum) to
check row_sum.is_finite() immediately after each addition (row_sum +=
row[c].abs()) and if it becomes non-finite return Err(LaError::non_finite_at(r,
c)) so callers receive both row and col metadata; ensure you use the existing
LaError::non_finite_at signature that includes (row, col) or adapt the call to
the correct two-argument constructor if present.
- Around line 602-609: The error path inside the nested loops over r and c
currently reports LaError::non_finite_at(c), losing the row coordinate; update
the non-finite error construction to include both row and column so callers like
first_asymmetry(), is_symmetric(), and Matrix::ldlt() receive full NonFinite {
row, col } metadata (i.e., call the two-argument/row-and-col variant of
non_finite_at or construct LaError::NonFinite with row=r and col=c), keeping the
same cold_path() early return behavior and using the existing symbols row_eps,
rel_tol, cold_path, and LaError::non_finite_at.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 26: Update the changelog sentence to use the current error name: replace
the mention of preserving `Overflow` for finite-f64 conversion failures with
`LaError::Unrepresentable` so it reflects the strict exact→f64 contract; locate
the line referencing `det_exact_f64` and change the error term to
`LaError::Unrepresentable`.
🪄 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: 73e68e8c-5d9b-41f1-8d57-fe751c6b2234
📒 Files selected for processing (25)
CHANGELOG.mdREADME.mdbenches/exact.rsdocs/BENCHMARKING.mddocs/archive/changelog/0.1.mddocs/archive/changelog/0.2.mddocs/archive/changelog/0.3.mdexamples/exact_solve_3x3.rsjustfilescripts/bench_compare.pyscripts/tests/test_bench_compare.pysemgrep.yamlsrc/error.rssrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/tolerance.rssrc/vector.rstests/proptest_exact.rstests/regressions.rstests/semgrep/src/project_rules/finite_api_contract.rstests/semgrep/src/project_rules/raw_f64_constructors.rstests/semgrep/src/project_rules/readme_doctest_mirrors.rs
💤 Files with no reviewable changes (4)
- tests/semgrep/src/project_rules/finite_api_contract.rs
- docs/archive/changelog/0.3.md
- docs/archive/changelog/0.2.md
- docs/archive/changelog/0.1.md
Comment on lines
+350
to
+363
| let mut max_row_sum: f64 = 0.0; | ||
|
|
||
| let mut r = 0; | ||
| while r < D { | ||
| let row = &self.rows[r]; | ||
| let mut row_sum: f64 = 0.0; | ||
| let mut c = 0; | ||
| while c < D { | ||
| row_sum += row[c].abs(); | ||
| c += 1; | ||
| } | ||
| if !row_sum.is_finite() { | ||
| cold_path(); | ||
| return Err(LaError::non_finite_at(r)); |
Contributor
There was a problem hiding this comment.
Preserve (row, col) metadata on inf_norm overflow.
Lines 358-363 wait until the end of the row and then return LaError::non_finite_at(r), so callers lose the column that first overflowed the accumulator. This regresses the crate's non-finite reporting contract for matrix operations.
As per coding guidelines, "Non-finite values (NaN, ±∞) always surface as LaError::NonFinite { row, col } with source-location metadata." Based on learnings, the same rule applies to src/**/*.rs.
Suggested fix
while r < D {
let row = &self.rows[r];
let mut row_sum: f64 = 0.0;
let mut c = 0;
while c < D {
row_sum += row[c].abs();
+ if !row_sum.is_finite() {
+ cold_path();
+ return Err(LaError::non_finite_cell(r, c));
+ }
c += 1;
}
- if !row_sum.is_finite() {
- cold_path();
- return Err(LaError::non_finite_at(r));
- }
if row_sum > max_row_sum {
max_row_sum = row_sum;
}
r += 1;
}🤖 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 350 - 363, The inf-norm accumulation currently
waits until the end of the row and calls LaError::non_finite_at(r), losing the
column index; change the inner loop in the method (the loops using variables r,
c and row_sum) to check row_sum.is_finite() immediately after each addition
(row_sum += row[c].abs()) and if it becomes non-finite return
Err(LaError::non_finite_at(r, c)) so callers receive both row and col metadata;
ensure you use the existing LaError::non_finite_at signature that includes (row,
col) or adapt the call to the correct two-argument constructor if present.
Sources: Coding guidelines, Learnings
Comment on lines
+602
to
+609
| for r in 0..D { | ||
| let mut row_eps = 0.0; | ||
| for c in 0..D { | ||
| row_eps = rel_tol.mul_add(self.rows[r][c].abs(), row_eps); | ||
| if !row_eps.is_finite() { | ||
| cold_path(); | ||
| return Err(LaError::non_finite_at(c)); | ||
| } |
Contributor
There was a problem hiding this comment.
Return matrix coordinates from symmetry-scale overflow paths.
Line 608 reports LaError::non_finite_at(c) even though this loop has both r and c. That drops the row metadata for overflows observed by first_asymmetry(), is_symmetric(), and Matrix::ldlt().
As per coding guidelines, "Non-finite values (NaN, ±∞) always surface as LaError::NonFinite { row, col } with source-location metadata." Based on learnings, the same rule applies to src/**/*.rs.
Suggested fix
for r in 0..D {
let mut row_eps = 0.0;
for c in 0..D {
row_eps = rel_tol.mul_add(self.rows[r][c].abs(), row_eps);
if !row_eps.is_finite() {
cold_path();
- return Err(LaError::non_finite_at(c));
+ return Err(LaError::non_finite_cell(r, c));
}
}
if row_eps > eps {
eps = row_eps;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for r in 0..D { | |
| let mut row_eps = 0.0; | |
| for c in 0..D { | |
| row_eps = rel_tol.mul_add(self.rows[r][c].abs(), row_eps); | |
| if !row_eps.is_finite() { | |
| cold_path(); | |
| return Err(LaError::non_finite_at(c)); | |
| } | |
| for r in 0..D { | |
| let mut row_eps = 0.0; | |
| for c in 0..D { | |
| row_eps = rel_tol.mul_add(self.rows[r][c].abs(), row_eps); | |
| if !row_eps.is_finite() { | |
| cold_path(); | |
| return Err(LaError::non_finite_cell(r, c)); | |
| } | |
| } | |
| if row_eps > eps { | |
| eps = row_eps; | |
| } | |
| } |
🤖 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 602 - 609, The error path inside the nested loops
over r and c currently reports LaError::non_finite_at(c), losing the row
coordinate; update the non-finite error construction to include both row and
column so callers like first_asymmetry(), is_symmetric(), and Matrix::ldlt()
receive full NonFinite { row, col } metadata (i.e., call the
two-argument/row-and-col variant of non_finite_at or construct
LaError::NonFinite with row=r and col=c), keeping the same cold_path() early
return behavior and using the existing symbols row_eps, rel_tol, cold_path, and
LaError::non_finite_at.
Sources: Coding guidelines, Learnings
- Return matrix-cell metadata when inf-norm row sums or symmetry tolerance scaling overflow. - Avoid reparsing finite-by-construction RHS vectors in LU and LDLT solves.
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
2 participants
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: det_exact_f64 and solve_exact_f64 now return LaError::Unrepresentable instead of rounding or reporting Overflow for exact values that cannot be represented exactly as finite f64. Call det_exact_rounded_f64 or solve_exact_rounded_f64 to opt into rounded finite f64 results.
BREAKING CHANGE: FiniteMatrix, FiniteVector, and Matrix::from_rows are removed from the public API. Use Matrix, Vector, Matrix::try_from_rows, and Vector::try_new instead.
Closes #148
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Bug Fixes
Tests