feat: const-ify Lu/Ldlt det + solve_vec and Matrix inf_norm + det_err…#95
feat: const-ify Lu/Ldlt det + solve_vec and Matrix inf_norm + det_err…#95acgetchell merged 3 commits intomainfrom
Conversation
…bound
Make six public methods `const fn`, completing const-evaluation parity
with `Matrix::det_direct` and `Vector::dot` now that MSRV 1.95 exposes
`f64::mul_add` as const (1.94) and `core::hint::cold_path` as const
(1.95):
- `Lu::det`, `Lu::solve_vec`
- `Ldlt::det`, `Ldlt::solve_vec`
- `Matrix::inf_norm`, `Matrix::det_errbound`
Iterator chains (`.iter().map().sum()`, `.enumerate().take(i)`,
`.enumerate().skip(i + 1)`, `.iter_mut().enumerate().take(D)`) were
rewritten as `while` loops since they are not const-stable.
Fix error-variant correctness in both solve_vec paths:
- A corrupt stored `U` / `D` diagonal at `(i, i)` now surfaces as
`LaError::NonFinite { row: Some(i), col: i }`, matching the
convention used by `Matrix::det`, `Lu::factor`, and `Ldlt::factor`.
- A computed-intermediate overflow keeps `row: None, col: i`.
- Previously both were conflated into `row: None, col: i`, defeating
debuggability for callers who construct factorizations directly.
Sharpen `LaError::NonFinite` variant docs and the `# Errors` sections of
`Lu::solve_vec` / `Ldlt::solve_vec` to spell out the `(row, col)`
contract for each failure mode.
Cleanup:
- Import `ERR_COEFF_{2,3,4}` at `src/matrix.rs` top; drop `crate::`
prefixes inside `Matrix::det_errbound`.
- Rename intermediate bindings `q` / `v` → `quotient` to stay under
`clippy::many_single_char_names`.
- Rename const-evaluability tests from `*_is_const_evaluable_*` to
`*_const_eval_*` for a concise, consistent style.
Tests:
- Added `{lu,ldlt,inf_norm,det_errbound}_const_eval_*` tests that force
compile-time evaluation inside `const { … }` initializers.
- Added `solve_vec_defensive_non_finite_diagonal_{2,3,4,5}d` in
`src/lu.rs` covering the new split error path (previously only the
`Singular` defensive path was exercised).
- Updated matching `Ldlt` defensive tests to assert the new
`row: Some(D - 1), col: D - 1` payload.
Validation: `just ci` passes — 164 unit tests, 324 integration/feature
tests, 28 doctests; clippy pedantic + nursery as errors; all examples.
Co-Authored-By: Oz <oz-agent@warp.dev>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 2 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughConverts several linear-algebra routines to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 #95 +/- ##
==========================================
+ Coverage 90.96% 94.74% +3.78%
==========================================
Files 5 5
Lines 498 514 +16
==========================================
+ Hits 453 487 +34
+ Misses 45 27 -18
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:
|
Synchronize the CHANGELOG, README, and REFERENCES for the upcoming release. This includes documenting the new absolute error bound coefficients for determinants, adding a dedicated section for AI agent governance, and refining internal error helpers to use a more consistent coordinate convention for non-finite values. Refs: #86
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/ldlt.rs (1)
561-610: Consider extending const-eval tests across D=2..5 via a macro.The const-evaluability tests here (
ldlt_det_const_eval_d2,_d3, andldlt_solve_vec_const_eval_d2) only cover a subset of dimensions, whereas the existinggen_public_api_ldlt_*andgen_solve_vec_defensive_testsmacros in this file already cover D=2..5. Since theconst fnlowering is identical acrossD, agen_ldlt_const_eval_tests!macro would keep coverage consistent with the rest of the module and the project convention without adding meaningful maintenance cost.As per coding guidelines: "Tests for dimension-generic code must cover D=2 through D=5 whenever possible using macros for per-dimension test generation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ldlt.rs` around lines 561 - 610, The const-eval tests only exercise D=2 and D=3 (and one solve_vec at D=2) but should cover D=2..5 like the other dimension-generic tests; add a macro (e.g., gen_ldlt_const_eval_tests!) that generates const-evaluability tests for Ldlt::<D>::det() and Ldlt::<D>::solve_vec(...) for D = 2..5, reuse the same test patterns in the existing ldlt_det_const_eval_d2/3 and ldlt_solve_vec_const_eval_d2 tests, and invoke the macro for the range 2..=5 so coverage matches the other macros (gen_public_api_ldlt_* / gen_solve_vec_defensive_tests) and ensures Ldlt and its const fns are exercised across dimensions.src/matrix.rs (1)
912-950: Consider extending const-eval tests to D=4 and D=5 (where applicable).
det_errbound_const_eval_*covers D=2,3 andinf_norm_const_eval_*covers D=2,3;det_direct_const_eval_*likewise.det_errboundadditionally has a meaningful D=4 branch (and D=5 →None) worth exercising underconst { … }, andinf_normis dimension-agnostic so macro-generated D=2..5 coverage would match the convention used bygen_public_api_matrix_tests!andgen_inf_norm_nonfinite_tests!elsewhere in this file.As per coding guidelines: "Tests for dimension-generic code must cover D=2 through D=5 whenever possible using macros for per-dimension test generation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/matrix.rs` around lines 912 - 950, Add const-eval tests for dimensions 4 and 5 to match guidelines: extend the det_errbound_const_eval_* and inf_norm_const_eval_* coverage to include Matrix::<4> and Matrix::<5> cases (noting det_errbound has a meaningful D=4 branch and returns None for D=5), using the same const { ... } pattern or the existing test-generation macro pattern (e.g., mirror gen_public_api_matrix_tests!/gen_inf_norm_nonfinite_tests! approach) so det_errbound(), inf_norm(), and det_direct_const_eval_* are exercised for D=2..5; update or add macro-driven tests that call Matrix::<4>::from_rows/... and Matrix::<5>::from_rows/... (or identity() where appropriate) and assert the expected Option or numeric results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ldlt.rs`:
- Around line 561-610: The const-eval tests only exercise D=2 and D=3 (and one
solve_vec at D=2) but should cover D=2..5 like the other dimension-generic
tests; add a macro (e.g., gen_ldlt_const_eval_tests!) that generates
const-evaluability tests for Ldlt::<D>::det() and Ldlt::<D>::solve_vec(...) for
D = 2..5, reuse the same test patterns in the existing ldlt_det_const_eval_d2/3
and ldlt_solve_vec_const_eval_d2 tests, and invoke the macro for the range 2..=5
so coverage matches the other macros (gen_public_api_ldlt_* /
gen_solve_vec_defensive_tests) and ensures Ldlt and its const fns are exercised
across dimensions.
In `@src/matrix.rs`:
- Around line 912-950: Add const-eval tests for dimensions 4 and 5 to match
guidelines: extend the det_errbound_const_eval_* and inf_norm_const_eval_*
coverage to include Matrix::<4> and Matrix::<5> cases (noting det_errbound has a
meaningful D=4 branch and returns None for D=5), using the same const { ... }
pattern or the existing test-generation macro pattern (e.g., mirror
gen_public_api_matrix_tests!/gen_inf_norm_nonfinite_tests! approach) so
det_errbound(), inf_norm(), and det_direct_const_eval_* are exercised for
D=2..5; update or add macro-driven tests that call Matrix::<4>::from_rows/...
and Matrix::<5>::from_rows/... (or identity() where appropriate) and assert the
expected Option or numeric results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1830124-f85b-4b2f-9338-3d9a673ea0d6
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdREFERENCES.mdsrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rstypos.toml
✅ Files skipped from review due to trivial changes (4)
- typos.toml
- README.md
- REFERENCES.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- src/lu.rs
Refactor manual const-evaluation tests in `Ldlt` and `Matrix` into macros to standardize coverage across matrix dimensions 2 through 5. This ensures all determinant and norm variants are fully exercised at compile time. Refs: #86
1 participant
…bound
Make six public methods
const fn, completing const-evaluation parity withMatrix::det_directandVector::dotnow that MSRV 1.95 exposesf64::mul_addas const (1.94) andcore::hint::cold_pathas const (1.95):Lu::det,Lu::solve_vecLdlt::det,Ldlt::solve_vecMatrix::inf_norm,Matrix::det_errboundIterator chains (
.iter().map().sum(),.enumerate().take(i),.enumerate().skip(i + 1),.iter_mut().enumerate().take(D)) were rewritten aswhileloops since they are not const-stable.Fix error-variant correctness in both solve_vec paths:
U/Ddiagonal at(i, i)now surfaces asLaError::NonFinite { row: Some(i), col: i }, matching the convention used byMatrix::det,Lu::factor, andLdlt::factor.row: None, col: i.row: None, col: i, defeating debuggability for callers who construct factorizations directly.Sharpen
LaError::NonFinitevariant docs and the# Errorssections ofLu::solve_vec/Ldlt::solve_vecto spell out the(row, col)contract for each failure mode.Cleanup:
ERR_COEFF_{2,3,4}atsrc/matrix.rstop; dropcrate::prefixes insideMatrix::det_errbound.q/v→quotientto stay underclippy::many_single_char_names.*_is_const_evaluable_*to*_const_eval_*for a concise, consistent style.Tests:
{lu,ldlt,inf_norm,det_errbound}_const_eval_*tests that force compile-time evaluation insideconst { … }initializers.solve_vec_defensive_non_finite_diagonal_{2,3,4,5}dinsrc/lu.rscovering the new split error path (previously only theSingulardefensive path was exercised).Ldltdefensive tests to assert the newrow: Some(D - 1), col: D - 1payload.Validation:
just cipasses — 164 unit tests, 324 integration/feature tests, 28 doctests; clippy pedantic + nursery as errors; all examples.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests