perf: stack-backed storage for exact arithmetic kernels#68
Merged
acgetchell merged 3 commits intomainfrom Apr 10, 2026
Merged
Conversation
Replace heap-allocated Vec<Vec<BigRational>> with stack-backed [[BigRational; D]; D] arrays in bareiss_det and gauss_solve, and return [BigRational; D] from gauss_solve directly — eliminating per-call heap traffic and a redundant clone in solve_exact. Exact kernel changes (src/exact.rs): - bareiss_det: Vec<Vec<BigRational>> → [[BigRational; D]; D] via std::array::from_fn - gauss_solve: Vec<Vec<BigRational>> augmented matrix → separate [[BigRational; D]; D] + [BigRational; D] (avoids unstable const-generic [T; D+1]) - gauss_solve return type: Vec<BigRational> → [BigRational; D] - solve_exact: drop intermediate clone, forward array directly Error enrichment (src/lib.rs, src/lu.rs, src/ldlt.rs, src/matrix.rs, src/exact.rs): - LaError::NonFinite gains row: Option<usize> for full (row, col) location on matrix inputs; None for vectors/intermediates - LaError::Overflow becomes a struct variant with index: Option<usize> to identify which solution component overflowed - Display messages updated to show positional context Benchmarks: - New benches/exact.rs: Criterion benchmarks for det, det_direct, det_exact, det_exact_f64, det_sign_exact, solve_exact, and solve_exact_f64 across D=2..5, plus near-singular 3×3 fallback - Cargo.toml: add [[bench]] entry with required-features = ["bench", "exact"] Closes #62
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors exact-arithmetic kernels to use fixed-size, stack-backed arrays, expands error payloads with row/index context across matrix routines, and adds Criterion benchmarks for exact arithmetic over D=2..5 (including a near-singular 3×3 case). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 #68 +/- ##
==========================================
- Coverage 93.31% 88.23% -5.09%
==========================================
Files 5 5
Lines 374 391 +17
==========================================
- Hits 349 345 -4
- Misses 25 46 +21
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lu.rs (1)
149-157:⚠️ Potential issue | 🔴 CriticalValidate the back-substitution quotient before storing it.
Line 157 assigns
sum / diagwithout checking the result. If that division overflows on the last row,solve_vec()returnsOk(...)with a non-finite component instead ofLaError::NonFinite.Suggested fix
if diag.abs() <= self.tol { return Err(LaError::Singular { pivot_col: i }); } - x[i] = sum / diag; + let v = sum / diag; + if !v.is_finite() { + return Err(LaError::NonFinite { row: None, col: i }); + } + x[i] = v; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lu.rs` around lines 149 - 157, The back-substitution in solve_vec currently assigns x[i] = sum / diag without validating the quotient; modify solve_vec (the back-substitution loop where diag, sum, and x are used) to compute the quotient let q = sum / diag, check q.is_finite() (or otherwise validate), and if not return Err(LaError::NonFinite { row: None, col: i }) before assigning to x[i]; keep existing checks for diag and sum beforehand and ensure the error uses pivot column i as in the other checks.src/ldlt.rs (1)
68-75:⚠️ Potential issue | 🟡 MinorReport the updated column here, not the pivot column.
new_valis the value forf.rows[i][k], but Line 74 reportscol: j. When this overflow trips off the pivot column, the new error metadata points at the wrong entry.Suggested fix
if !new_val.is_finite() { return Err(LaError::NonFinite { row: Some(i), - col: j, + col: k, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ldlt.rs` around lines 68 - 75, The NonFinite error is reporting the pivot column j instead of the updated column k where new_val is assigned; update the error metadata in the block inside the loop that computes new_val so LaError::NonFinite uses col: k (and row: Some(i) stays the same) — locate the loop in ldlt.rs that computes new_val (uses variables i, j, k, l_k, l_i_d and f.rows[i][k]) and change the error construction to reference k instead of j.src/matrix.rs (1)
294-300:⚠️ Potential issue | 🟠 MajorPreserve matrix coordinates in the direct-det non-finite path.
Line 299 currently collapses every
det_direct()failure intoNonFinite { row: None, col: 0 }. For D ≤ 4, a NaN/∞ input matrix now loses its(row, col)location, while the D ≥ 5 LU path still reports the offending matrix entry. That makesMatrix::det’s public error shape depend on dimension.Suggested fix
pub fn det(self, tol: f64) -> Result<f64, LaError> { if let Some(d) = self.det_direct() { - return if d.is_finite() { - Ok(d) - } else { - Err(LaError::NonFinite { row: None, col: 0 }) - }; + if d.is_finite() { + return Ok(d); + } + + for r in 0..D { + for c in 0..D { + if !self.rows[r][c].is_finite() { + return Err(LaError::NonFinite { + row: Some(r), + col: c, + }); + } + } + } + + return Err(LaError::NonFinite { row: None, col: 0 }); } self.lu(tol).map(|lu| lu.det()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/matrix.rs` around lines 294 - 300, The det method currently turns any non-finite result from det_direct() into LaError::NonFinite { row: None, col: 0 }, losing the offending coordinate; instead, when det_direct() returns a non-finite determinant, scan the matrix for the first non-finite entry and return Err(LaError::NonFinite { row: Some(r), col: c }) so the error preserves the (row,col); update the det function (the branch handling det_direct() in Matrix::det) to perform this quick nested-loop check over the matrix elements and populate the LaError::NonFinite fields rather than using row: None, col: 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ldlt.rs`:
- Around line 68-75: The NonFinite error is reporting the pivot column j instead
of the updated column k where new_val is assigned; update the error metadata in
the block inside the loop that computes new_val so LaError::NonFinite uses col:
k (and row: Some(i) stays the same) — locate the loop in ldlt.rs that computes
new_val (uses variables i, j, k, l_k, l_i_d and f.rows[i][k]) and change the
error construction to reference k instead of j.
In `@src/lu.rs`:
- Around line 149-157: The back-substitution in solve_vec currently assigns x[i]
= sum / diag without validating the quotient; modify solve_vec (the
back-substitution loop where diag, sum, and x are used) to compute the quotient
let q = sum / diag, check q.is_finite() (or otherwise validate), and if not
return Err(LaError::NonFinite { row: None, col: i }) before assigning to x[i];
keep existing checks for diag and sum beforehand and ensure the error uses pivot
column i as in the other checks.
In `@src/matrix.rs`:
- Around line 294-300: The det method currently turns any non-finite result from
det_direct() into LaError::NonFinite { row: None, col: 0 }, losing the offending
coordinate; instead, when det_direct() returns a non-finite determinant, scan
the matrix for the first non-finite entry and return Err(LaError::NonFinite {
row: Some(r), col: c }) so the error preserves the (row,col); update the det
function (the branch handling det_direct() in Matrix::det) to perform this quick
nested-loop check over the matrix elements and populate the LaError::NonFinite
fields rather than using row: None, col: 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e18022f-e746-450e-9a0a-a2793cc6d318
📒 Files selected for processing (7)
Cargo.tomlbenches/exact.rssrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rs
Refine error reporting by ensuring correct column indices in LDLT decomposition and LU back-substitution. Add missing finiteness checks to LU solve paths and update determinant calculations to provide specific row/column coordinates for non-finite inputs. Refs: #62
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.
Replace heap-allocated Vec<Vec> with stack-backed [[BigRational; D]; D] arrays in bareiss_det and gauss_solve, and return [BigRational; D] from gauss_solve directly — eliminating per-call heap traffic and a redundant clone in solve_exact.
Exact kernel changes (src/exact.rs):
Error enrichment (src/lib.rs, src/lu.rs, src/ldlt.rs, src/matrix.rs, src/exact.rs):
to identify which solution component overflowed
Benchmarks:
Closes #62
Summary by CodeRabbit
New Features
Bug Fixes