perf(matrix): restore det_direct throughput for D=2..4#181
Conversation
- Reintroduce branch-free dense expansions for D=2 and D=3. - Share D=4 minors while retaining guarded sparse evaluation. - Preserve non-finite handling for mathematically inactive terms. Closes #158
📝 WalkthroughWalkthroughDeterminant evaluation for fixed-size matrices adds dense D=3 and D=4 paths, adjusts D=2 underflow tracking, updates determinant documentation, adds focused tests, and expands repository review configuration. ChangesFixed-size determinant paths
Review configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Matrix
participant det_direct
participant det_direct_with_errbound
participant det3_det4_helpers
Matrix->>det_direct: evaluate fixed-size determinant
det_direct->>det3_det4_helpers: select dense or guarded cofactor path
det3_det4_helpers-->>det_direct: return determinant
Matrix->>det_direct_with_errbound: evaluate determinant with error bound
det_direct_with_errbound-->>Matrix: return matching determinant and bound
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 8 8
Lines 4624 4684 +60
=======================================
+ Hits 4520 4579 +59
- Misses 104 105 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![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/matrix.rs (2)
955-984: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFormula verified correct; consider documenting the rounding-error-bound status.
Verified
det4_dense_elementsalgebraically against the standard 4×4 Laplace expansion (M00..M03 via shared 2×2 minorss01..s23) and against the test's hand-computed expected value (112.0) — the FMA chain is correct.One documentation gap: per coding guidelines, any
f64operation that can accumulate rounding error should either cite its absolute bound (ERR_COEFF_*) or explicitly state that no bound is provided. This new hot-path function's doc explains the NaN-avoidance invariant but doesn't mention its own rounding-error characterization (it's covered indirectly viaERR_COEFF_4/det_errbound, but that connection isn't stated here).As per coding guidelines: "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."📝 Suggested doc addition
/// Callers must establish that the first two rows contain no zero /// coefficients. That proof makes every minor part of an active Leibniz /// term, so evaluating all six shared minors cannot introduce a non-finite /// result through a mathematically absent term. + /// + /// This routine does not itself certify a rounding-error bound; callers + /// needing one should pair this result with [`ERR_COEFF_4`](crate::ERR_COEFF_4) + /// via [`det_errbound`](Self::det_errbound).🤖 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 955 - 984, Update the documentation for det4_dense_elements to explicitly state its rounding-error characterization, referencing the existing ERR_COEFF_4/det_errbound bound if applicable; otherwise state that no absolute bound is provided. Preserve the existing explanation of the NaN-avoidance invariant.Source: Coding guidelines
1787-1834: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: mirror the D=4 dense test for D=3.
det3_elements's new dense fast path is only incidentally exercised today (e.g.,det_direct_d3_known_value's matrix happens to have a fully non-zero first row, and its expected determinant is0.0, a less illuminating check of the FMA chain). Consider adding a dedicateddet_direct_d3_dense_known_valuetest with a non-singular, fully-dense matrix and a hand-verified nonzero expected value, mirroringdet_direct_d4_dense_known_value, for symmetry and to make the D=3 dense-path intent explicit in the test suite.🤖 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 1787 - 1834, Add a dedicated det_direct_d3_dense_known_value test alongside det_direct_d3_known_value, using a fully dense, non-singular 3×3 matrix with a hand-verified nonzero determinant. Mirror det_direct_d4_dense_known_value by comparing det_direct() and det_direct_with_errbound() results, ensuring the D=3 dense det3_elements fast path is explicitly exercised.
🤖 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.
Nitpick comments:
In `@src/matrix.rs`:
- Around line 955-984: Update the documentation for det4_dense_elements to
explicitly state its rounding-error characterization, referencing the existing
ERR_COEFF_4/det_errbound bound if applicable; otherwise state that no absolute
bound is provided. Preserve the existing explanation of the NaN-avoidance
invariant.
- Around line 1787-1834: Add a dedicated det_direct_d3_dense_known_value test
alongside det_direct_d3_known_value, using a fully dense, non-singular 3×3
matrix with a hand-verified nonzero determinant. Mirror
det_direct_d4_dense_known_value by comparing det_direct() and
det_direct_with_errbound() results, ensuring the D=3 dense det3_elements fast
path is explicitly exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d9f8e19-fd55-4ea7-8187-78baf9d27f44
📒 Files selected for processing (2)
src/lib.rssrc/matrix.rs
- Use the legacy required status while preserving automatic approvals. - Add explicit dense D3 determinant coverage and document the D4 error bound.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/matrix.rs (2)
955-990: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
det4_dense_elementsis generic overDbut only valid forD == 4.I hand-derived the full expansion and confirmed the arithmetic is bit-for-bit identical to the guarded path when all row-0/row-1 coefficients are non-zero — the computation itself is correct.
However, the signature takes
&[[f64; D]; D](the enclosing impl's generic dimension) rather than a fixed&[[f64; 4]; 4], unlikedet3_elements, which takes literal[f64; 3]rows and is therefore dimensionally self-evident regardless of callerD. SinceSelf::det4_dense_elementsis a private helper reachable from anyDmonomorphization, a future refactor that mistakenly calls it outside the4 =>arm (e.g., if aD=5+ closed form is ever added) would silently compute a determinant from only the top-left 4×4 block instead of panicking or erroring — a silent wrong-answer bug rather than a caught one.A cheap safeguard is a debug-only precondition check, consistent with how this codebase already reserves panics for debug-only precondition violations (e.g., the LDLT symmetry check).
🛡️ Proposed defensive check
#[inline(always)] const fn det4_dense_elements(r: &[[f64; D]; D]) -> f64 { + debug_assert!(D == 4, "det4_dense_elements requires D == 4"); let s23 = r[2][2].mul_add(r[3][3], -(r[2][3] * r[3][2]));As per coding guidelines,
src/**/*.rsshould "Use const-genericDfor every core type... No runtime dimension," which this helper's untypedD-generic signature only partially honors since it relies on caller discipline rather than the type system.🤖 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 955 - 990, Add a debug-only dimension precondition to det4_dense_elements, asserting that the enclosing const generic D equals 4 before evaluating the expansion. Keep the existing arithmetic unchanged and preserve release behavior, while making misuse from other D monomorphizations fail during debugging instead of silently using the top-left 4×4 block.Source: Coding guidelines
1808-1823: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an adversarial/large-magnitude test for the new dense paths.
The new D3/D4 dense-path tests (
det_direct_d3_dense_known_value,det_direct_d4_dense_known_value) only cover well-conditioned, small-integer matrices. The existing adversarial tests (1e300-scale entries) all target the sparse/guarded branch, since they rely on zero coefficients to trigger skip logic — none exercisedet3_elements's ordet4_dense_elements's new dense branch with large-magnitude, fully non-zero entries (e.g., a dense matrix near the overflow boundary) to confirm legitimate overflow is still correctly surfaced asLaError::NonFiniterather than silently mishandled by the new shared-minor arithmetic.As per coding guidelines,
{src/**/*.rs,benches/**/*.rs,tests/**/*.rs}: "Adversarial inputs (near-singular, large-entry, Hilbert-style ill-conditioning) should accompany well-conditioned inputs in both tests and benchmarks."🤖 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 1808 - 1823, Extend the dense determinant coverage around det_direct_d3_dense_known_value and det_direct_d4_dense_known_value with fully non-zero, large-magnitude matrices near the overflow boundary. Exercise det3_elements and det4_dense_elements directly through the public determinant paths, and assert that legitimate overflow is reported as LaError::NonFinite rather than producing an invalid or silent result; retain the existing well-conditioned cases.Source: Coding guidelines
🤖 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 @.coderabbit.yaml:
- Around line 8-10: Replace the unsupported review_progress setting in the
review configuration with reviews.review_status set to false, preserving the
existing behavior of disabling the review-progress check run.
---
Nitpick comments:
In `@src/matrix.rs`:
- Around line 955-990: Add a debug-only dimension precondition to
det4_dense_elements, asserting that the enclosing const generic D equals 4
before evaluating the expansion. Keep the existing arithmetic unchanged and
preserve release behavior, while making misuse from other D monomorphizations
fail during debugging instead of silently using the top-left 4×4 block.
- Around line 1808-1823: Extend the dense determinant coverage around
det_direct_d3_dense_known_value and det_direct_d4_dense_known_value with fully
non-zero, large-magnitude matrices near the overflow boundary. Exercise
det3_elements and det4_dense_elements directly through the public determinant
paths, and assert that legitimate overflow is reported as LaError::NonFinite
rather than producing an invalid or silent result; retain the existing
well-conditioned cases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70d9fca5-c3e8-4f18-92fe-6414574a5c69
📒 Files selected for processing (2)
.coderabbit.yamlsrc/matrix.rs
1 participant
Closes #158
Summary by CodeRabbit