-
Notifications
You must be signed in to change notification settings - Fork 0
Changed: Overhauls public API tests and examples #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Modernizes tests by adding property-based tests and tests for edge cases. Replaces examples with larger sizes to demonstrate pivoting. This change focuses on the tests and examples, improving coverage and demonstrating common usage scenarios.
WalkthroughAdds new 5×5 examples, replaces 3×3 examples, adds dev-dependencies (proptest, pastey), expands and macro-parameterizes unit tests across src/, adds property-based tests for Matrix and Vector, updates docs/CI/metadata and .gitignore, and extends crate-level doctests and unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/proptest_vector.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
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 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
WARP.md (1)
24-24: Docs: example reference now correctly targets the 5×5 determinant demoThe updated
cargo run --example det_5x5matches the new example and theexamplesrecipe. Optionally, you could also mentionsolve_5x5here for symmetry with thejust examplestarget, but not required.src/lu.rs (1)
183-260: Pivoting public-API tests look solid; RHS generation could be made dimension-agnosticThese macro-generated tests nicely exercise the public paths
Matrix::lu→Lu::solve_vec/Lu::det, useblack_boxon both the function pointers and data, and correctly validate:
- Row-swap pivoting for a permutation matrix that swaps the first two basis vectors.
- That
solve_veceffectively inverts this permutation on a simple RHS.One small maintainability tweak:
values = [1.0, 2.0, 3.0, 4.0, 5.0]is hard-coded to length 5. Today you only instantiate the macro forD ∈ {2,3,4,5}, so it’s fine, but if someone later extends it to largerD, the tail entries ofb_arrwill remain0.0.You could make this dimension-agnostic with something like:
let b_arr = { let mut arr = [0.0f64; $d]; for (i, dst) in arr.iter_mut().enumerate() { *dst = (i + 1) as f64; } arr };so the macro remains correct for any future
$d.tests/proptest_vector.rs (1)
13-60: Consider using a small epsilon for dot product comparisons.The property-based tests are well-designed and cover important mathematical properties. However, lines 45 and 55 use
epsilon = 0.0for dot product comparisons. While this may work with the limited input range (-100.0 to 100.0), floating-point arithmetic can introduce rounding errors.Consider using a small epsilon for robustness:
- assert_abs_diff_eq!(dot_ab, dot_ba, epsilon = 0.0); + assert_abs_diff_eq!(dot_ab, dot_ba, epsilon = 1e-14);- assert_abs_diff_eq!(a.dot(z), 0.0, epsilon = 0.0); + assert_abs_diff_eq!(a.dot(z), 0.0, epsilon = 1e-14);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignore(1 hunks)Cargo.toml(2 hunks)README.md(5 hunks)WARP.md(1 hunks)cspell.json(2 hunks)examples/det_3x3.rs(0 hunks)examples/det_5x5.rs(1 hunks)examples/solve_3x3.rs(0 hunks)examples/solve_5x5.rs(1 hunks)justfile(3 hunks)src/lib.rs(2 hunks)src/lu.rs(1 hunks)src/matrix.rs(1 hunks)src/vector.rs(1 hunks)tests/proptest_matrix.rs(1 hunks)tests/proptest_vector.rs(1 hunks)
💤 Files with no reviewable changes (2)
- examples/solve_3x3.rs
- examples/det_3x3.rs
🧰 Additional context used
🧬 Code graph analysis (8)
examples/det_5x5.rs (2)
src/matrix.rs (3)
from_rows(25-27)lu(165-167)det(186-188)src/lu.rs (1)
det(169-175)
tests/proptest_matrix.rs (2)
tests/proptest_vector.rs (1)
small_f64(9-11)src/matrix.rs (7)
from_rows(25-27)get(84-90)zero(40-44)set(107-114)inf_norm(128-139)det(186-188)lu(165-167)
src/vector.rs (1)
src/matrix.rs (1)
zero(40-44)
src/matrix.rs (2)
src/vector.rs (3)
zero(37-39)new(22-24)into_array(70-72)src/lu.rs (2)
det(169-175)solve_vec(109-149)
examples/solve_5x5.rs (3)
examples/det_5x5.rs (1)
main(5-22)src/matrix.rs (2)
from_rows(25-27)lu(165-167)src/vector.rs (1)
new(22-24)
src/lu.rs (2)
src/matrix.rs (3)
from_rows(25-27)lu(165-167)det(186-188)src/vector.rs (2)
new(22-24)into_array(70-72)
tests/proptest_vector.rs (1)
src/vector.rs (6)
new(22-24)as_array(53-55)into_array(70-72)dot(87-95)norm2_sq(109-111)zero(37-39)
src/lib.rs (2)
src/matrix.rs (1)
identity(59-69)src/vector.rs (1)
new(22-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: audit
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (29)
.gitignore (1)
3-6: Ignore patterns align with new coverage and build artifacts
/coveragematches the tarpaulin XML output,build*andcobertura.xmlcover common local artifacts, and.DS_Storeis a harmless quality-of-life ignore. No issues.cspell.json (1)
9-9: Spell-check dictionary correctly extended for new tests and toolingThe added terms (doctests, laerror, nonfinite, pastey, proptest, proptests, tridiagonal) match the new test/tooling vocabulary and keep the list ordered. Looks good.
Also applies to: 18-18, 25-26, 30-31, 35-35
examples/det_5x5.rs (1)
1-22: 5×5 determinant example is mathematically and API-wise consistentThe matrix really is
J - Ianddet(J - I) = (D - 1) * (-1)^(D-1) = 4forD = 5, and the example exercisesMatrix::<5>::from_rows,lu(DEFAULT_PIVOT_TOL), andLu::detthrough the public API. No changes needed.Cargo.toml (1)
8-8: Manifest updates are consistent with the new test/bench setupAdding
readme = "README.md"is standard, and the dev-dependency versions line up with the new macro/property-based tests and benches. Please just confirm these crate versions are compatible withrust-version = "1.92"and edition 2024 in your CI matrix.Also applies to: 17-21
examples/solve_5x5.rs (1)
1-24: 5×5 solve example correctly demonstrates pivoting and solve_vec
Aandbare consistent with the comment (x = [1,2,3,4,5]⇒b = [14,13,12,11,10]), anda[0][0] = 0indeed forces a pivot in column 0. The example cleanly exercisesMatrix::lu,Lu::solve_vec, andVector::<5>::newfrom the prelude.justfile (1)
45-45: CI and workflow recipes now cover benches and 5×5 examples
ciandcommit-checknow includebench-compile, which is a nice safety net without running benches, and theexamplesrecipe correctly runsdet_5x5andsolve_5x5. Dependency ordering looks sane and non-cyclic.Also applies to: 57-57, 103-105
src/lu.rs (2)
261-335: Tridiagonal smoke tests correctly encode the knowndet = D + 1identityThe tridiagonal matrix construction (2 on the diagonal, −1 on sub/super-diagonals) is correct, and:
bbuilt as[1, 0, ..., 0, 1]really corresponds tox = [1, ..., 1]for this SPD system, so asserting everyx_i ≈ 1.0is mathematically sound.- The determinant check
det ≈ f64::from($d) + 1.0matches the closed-form formula for this Toeplitz tridiagonal, with reasonable epsilons forD = 16, 32, 64.These tests give good large-D coverage of both
solve_vecanddetwithout depending on nalgebra at runtime.
337-448: 1×1/2×2 basics plus singular/non-finite tests give strong edge-case coverageThe additional tests in this block look well thought out:
solve_1x1,solve_2x2_basic, anddet_2x2_basicuseblack_boxand function pointers to exercise bothLu::factorand the publicMatrix::lu,Lu::solve_vec, andLu::detpaths.det_requires_pivot_signandsolve_requires_pivotingcorrectly use a 2×2 permutation matrix to prove that pivoting andpiv_signare wired into the determinant and solve logic.singular_detectedandsingular_due_to_tolerance_at_first_pivotvalidate both exact singularity and tolerance-based singular detection with precisepivot_colexpectations.- The non-finite tests (NaN/∞ in pivot entry, pivot column scan, and the overflow scenarios in forward and back substitution) line up with the control-flow in
factorandsolve_vec, and the assertedpivot_colindices match whereLaError::NonFiniteis actually raised.Overall, this is a very thorough set of behavioral tests around the LU error paths; no changes needed.
src/vector.rs (2)
125-128: LGTM!The imports are well-chosen:
black_boxprevents constant-folding for better coverage metrics,approxprovides robust floating-point assertions, andpasteyenables macro-based test name generation.
229-232: LGTM!The macro invocations for dimensions 2-5 provide comprehensive coverage and align with the property-based tests in
tests/proptest_vector.rs.src/lib.rs (3)
3-3: LGTM!Including the README in crate documentation is a best practice that ensures docs.rs displays comprehensive usage information.
5-32: LGTM!The doctest provides an excellent demonstration of LU decomposition with pivoting. The 5×5 system (J - I matrix) is mathematically correct and showcases the library's ability to handle cases where the initial pivot is zero.
90-132: LGTM!The unit tests provide solid coverage of constants, error formatting, trait implementations, and prelude exports. The integration test at lines 123-131 ensures all prelude items work together correctly.
tests/proptest_matrix.rs (6)
9-15: LGTM!The strategy functions provide well-bounded test inputs (-100.0 to 100.0) that avoid numerical overflow while ensuring diverse test coverage. The
small_nonzero_f64strategy correctly excludes zero for diagonal matrices.
17-40: LGTM!The roundtrip test thoroughly validates
from_rowsandget, including proper boundary checking. The out-of-bounds validation at lines 38-39 ensures the API contract is enforced.
42-51: LGTM!The set/get test correctly validates in-bounds mutation behavior. Using
epsilon = 0.0is appropriate here since the value is stored and retrieved without arithmetic operations.
53-68: LGTM!The infinity norm test correctly validates that the implementation matches the mathematical definition (maximum absolute row sum) and enforces non-negativity.
70-100: LGTM!The diagonal matrix test is an excellent property-based validation: the determinant is the product of diagonal elements, and the solution is element-wise division. The tolerance of
1e-12is appropriate for LU decomposition operations.
107-110: LGTM!The macro invocations for dimensions 2-5 provide comprehensive test coverage and align with the testing strategy across the codebase.
README.md (5)
5-5: LGTM!The relative path
./LICENSEis more explicit and ensures proper linking across different documentation contexts.
14-15: LGTM!The expanded introduction clearly communicates the crate's purpose and design philosophy, providing valuable context for potential users.
49-73: LGTM!The upgraded 5×5 example effectively demonstrates LU decomposition with pivoting. The J - I matrix (zeros on diagonal) is an excellent showcase for the library's pivot handling. The mathematical correctness was verified, and the tolerance of
1e-12is appropriate.
93-94: LGTM!The example commands correctly reference the new 5×5 examples, maintaining consistency with the codebase updates.
111-111: LGTM!The license link update matches the badge update at line 5, ensuring consistency throughout the document.
src/matrix.rs (3)
203-204: LGTM!The imports are appropriate for macro-based test generation with floating-point comparisons.
206-295: LGTM!The macro generates comprehensive tests covering construction, bounds checking, norms, and LU operations. The tolerance choices are appropriate:
epsilon = 0.0for exact values andepsilon = 1e-12for numerical operations.
298-301: LGTM!The macro invocations for dimensions 2-5 provide comprehensive coverage and maintain consistency with the testing strategy across the codebase.
tests/proptest_vector.rs (2)
9-11: LGTM!The strategy function provides well-bounded test inputs that avoid numerical overflow while ensuring diverse test coverage, consistent with
tests/proptest_matrix.rs.
63-66: LGTM!The macro invocations for dimensions 2-5 provide comprehensive test coverage and maintain consistency with the testing strategy across the entire codebase.
Updates tests to use a smaller epsilon value (1e-14) when comparing floating-point numbers, improving numerical stability and preventing false negatives. Also, refactors test setup in `lu.rs` for clarity.
Modernizes tests by adding property-based tests and tests for edge cases. Replaces examples with larger sizes to demonstrate pivoting.
This change focuses on the tests and examples, improving coverage and demonstrating common usage scenarios.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.