Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes determinant computation in lambda-rs matrix math by replacing factorial-time Laplace expansion with Gaussian elimination (partial pivoting), adding allocation-free fast paths for common transform sizes.
Changes:
- Implement Gaussian-elimination-based determinant with partial pivoting.
- Add stack-allocated determinant paths for 3x3 and 4x4 matrices.
- Switch the larger-matrix scratch space to a contiguous row-major
Vec<f32>buffer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/lambda-rs/src/math/matrix.rs
Outdated
| if pivot_abs <= f32::EPSILON { | ||
| return 0.0; | ||
| } |
There was a problem hiding this comment.
The singular-pivot detection uses an absolute threshold pivot_abs <= f32::EPSILON, which will incorrectly return 0.0 for valid matrices whose pivots are small in magnitude (e.g., a diagonal matrix with leading entry 1e-8 has a non-zero determinant but would be treated as singular). Consider checking pivot_abs == 0.0 (or using a relative tolerance based on matrix scale) so non-zero determinants aren’t collapsed to zero.
crates/lambda-rs/src/math/matrix.rs
Outdated
| if pivot_abs <= f32::EPSILON { | ||
| return Ok(0.0); | ||
| } |
There was a problem hiding this comment.
The fallback elimination path also treats pivot_abs <= f32::EPSILON as singular. This is an absolute threshold and can make determinant() return 0.0 for non-singular matrices with small-magnitude values. Use an exact-zero check or a relative tolerance derived from the matrix magnitude to avoid incorrect zero determinants.
| if rows == 4 { | ||
| let mut data = [[0.0_f32; 4]; 4]; | ||
| for (i, row) in data.iter_mut().enumerate() { | ||
| for (j, value) in row.iter_mut().enumerate() { | ||
| *value = self.at(i, j); | ||
| } | ||
| } | ||
| return Ok(determinant_gaussian_stack::<4>(data)); | ||
| } |
There was a problem hiding this comment.
New 4x4 stack fast-path and the >4 generic elimination path aren’t covered by tests in this module (current determinant tests only exercise 2x2 and 3x3). Please add tests for at least one 4x4 determinant and one larger matrix (e.g., 5x5) to validate correctness and pivoting/sign behavior.
✅ Coverage Report📊 View Full HTML Report (download artifact) Overall Coverage
Changed Files in This PR
PR Files Coverage: 88.05% (413/469 lines) Generated by cargo-llvm-cov · Latest main coverage Last updated: 2026-03-12 20:36:18 UTC · Commit: |
Summary
Optimize determinant computation in
lambda-rsmatrix math by replacing recursive Laplace expansion with Gaussian elimination plus partial pivoting. This reduces determinant evaluation from factorial-time growth to cubic-time growth for square matrices and adds stack-allocated fast paths for common 3x3 and 4x4 cases.Related Issues
None linked.
Changes
crates/lambda-rs/src/math/matrix.rswith Gaussian elimination and partial pivoting.Vec<Vec<f32>>to a contiguous row-majorVec<f32>for better cache locality and fewer allocations.Type of Change
Affected Crates
lambda-rslambda-rs-platformlambda-rs-argslambda-rs-loggingChecklist
cargo +nightly fmt --all)cargo clippy --workspace --all-targets -- -D warnings)cargo test --workspace)Testing
Commands run:
cargo test -p lambda-rs math::matrixManual verification steps (if applicable):
determinant()uses stack fast paths for 3x3 and 4x4 matrices.Screenshots/Recordings
Not applicable.
Platform Testing
Additional Notes
Asymptotic runtime for determinant computation improves from
O(n!)under recursive Laplace expansion toO(n^3)with Gaussian elimination. Space remainsO(n^2), with lower constant overhead for both fixed-size and larger-matrix paths.