test: add adversarial-input coverage for exact arithmetic (#80)#97
Merged
acgetchell merged 4 commits intomainfrom Apr 21, 2026
Merged
test: add adversarial-input coverage for exact arithmetic (#80)#97acgetchell merged 4 commits intomainfrom
acgetchell merged 4 commits intomainfrom
Conversation
Expand benchmark, unit-test, and proptest coverage of the exact-arithmetic
APIs to catch tail cases that fixed well-conditioned inputs miss.
Benchmarks (benches/exact.rs):
- Factor out `bench_extreme_group` helper running the same four benches
(`det_sign_exact`, `det_exact`, `solve_exact`, `solve_exact_f64`) so
adversarial groups are directly comparable.
- Extend `exact_near_singular_3x3` with the two solve benches (the
primary motivating use case for exact solve was previously unmeasured).
- Add `exact_large_entries_3x3` (entries near `f64::MAX / 2`) to stress
intermediate BigInt growth during Bareiss forward elimination.
- Add `exact_hilbert_4x4` / `exact_hilbert_5x5` to stress the
`f64_decompose → BigInt` scaling path on ill-conditioned inputs.
- Tighten bench `expect(...)` messages to name the invariant each call
relies on (e.g. "non-singular matrix with finite entries") so panics
identify both where (Criterion bench name) and why.
Unit tests (src/exact.rs):
- `solve_exact_near_singular_3x3_integer_x0` — integer-x0 round-trip
through the 2^-50-perturbed matrix.
- `solve_exact_large_entries_3x3_unit_vector` — `A·[1,0,0] = [big,1,1]`
round-trip with `f64::MAX/2` diagonal.
- `det_sign_exact_large_entries_3x3_positive` — asserts the fast filter
falls through (`det_direct` is non-finite) and `det_exact_f64` returns
`Overflow { index: None }`.
- `det_sign_exact_hilbert_positive_{3,4,5}d` — Hilbert is SPD, sign = 1.
- `solve_exact_hilbert_residual_{3,4,5}d` — residual `A·x - b` is exactly
zero in `BigRational`, stronger than integer round-trips since Hilbert
entries are non-terminating in binary.
Proptests (tests/proptest_exact.rs):
- `solve_exact_integer_roundtrip_{2..5}d` — random diagonally-dominant
integer `A` and small-integer `x0`, verify `solve(A, A·x0) == x0` exactly.
- `solve_exact_residual_{2..5}d` — random `A` + small-integer `b`, verify
`A · solve(A, b) == b` exactly (catches back-sub bugs on fractional
solutions).
- `det_sign_exact_agrees_with_det_exact_{2..5}d` — on full (non-diagonal)
small-integer matrices, asserts `det_sign_exact() == det_exact().sign()`
(exercises the filter/fallback boundary previously only diagonal-tested).
Prelude (src/lib.rs):
- Re-export `BigInt` alongside `BigRational` (crate root + prelude).
- Re-export `FromPrimitive`, `Signed`, `ToPrimitive` from `num-traits` in
the prelude so the re-exported `BigRational` is actually usable for
construction (`from_f64`, `from_i64`) and sign queries without forcing
downstream users to add `num-bigint` / `num-rational` / `num-traits` to
their own Cargo.toml. Additive; no public API breaks.
Tooling (scripts/bench_compare.py + test_bench_compare.py):
- Register the three new adversarial groups in `EXACT_GROUPS`.
- Extend `_group_heading` with human-readable titles
("Large entries 3x3", "Hilbert 4x4", "Hilbert 5x5").
- Add group-heading unit tests for the new cases.
Test results (`just ci`):
- cargo test --features exact: 368 lib + 20 proptest_exact + 40 other
proptests + 34 doc-tests — all pass
- cargo test (no features): 175 lib + 40 proptests + 29 doc-tests — all pass
- Python: 104 tests pass (ty, mypy, ruff clean)
- Clippy (pedantic + nursery + cargo, `-D warnings`): clean
- fmt, taplo, yamllint, shellcheck, spell-check, bench-compile, examples: clean
Closes #80
Co-Authored-By: Oz <oz-agent@warp.dev>
Contributor
|
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 46 minutes and 27 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. 📝 WalkthroughWalkthroughAdded adversarial exact-arithmetic benchmarks (near-singular, large-entry, Hilbert), a grouped benchmarking helper that runs four exact benches per input, matching benchmark-comparison script updates, expanded tests (unit, regression, and proptest) exercising exact solve/determinant codepaths, and broadened the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 93.16% 91.40% -1.76%
==========================================
Files 5 5
Lines 512 512
==========================================
- Hits 477 468 -9
- Misses 35 44 +9
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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/exact.rs`:
- Around line 2116-2118: The Hilbert test coverage is missing D=2; add the D=2
invocation for the macro gen_det_sign_exact_hilbert_positive_tests! so the
per-dimension tests cover 2..=5; specifically insert
gen_det_sign_exact_hilbert_positive_tests!(2); alongside the existing
invocations at the locations where
gen_det_sign_exact_hilbert_positive_tests!(3);
gen_det_sign_exact_hilbert_positive_tests!(4);
gen_det_sign_exact_hilbert_positive_tests!(5); appear (and likewise add the D=2
invocation at the second similar block around the other occurrence of those
three lines referenced in the comment).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 487ac7c3-9a0c-49e7-9596-42bae578ca82
📒 Files selected for processing (6)
benches/exact.rsscripts/bench_compare.pyscripts/tests/test_bench_compare.pysrc/exact.rssrc/lib.rstests/proptest_exact.rs
Update the `exact` feature to re-export `BigInt` and essential `num-traits` (FromPrimitive, ToPrimitive, Signed) alongside `BigRational`. This simplifies downstream usage by removing the need for direct dependencies on the `num` ecosystem for common operations. Additionally, formalize adversarial-input benchmarking for near-singular, large-entry, and Hilbert matrices to better track performance on ill-conditioned data. CI benchmark comparisons are now more resilient to newly added benchmarks via `--baseline-lenient`. Refs: #80
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/proptest_exact.rs (1)
33-35:⚠️ Potential issue | 🟠 MajorFix the diagonal-dominance shift to match the generated off-diagonal range.
Line 69 still shifts by
D * 5 + 1, but lines 34-35 allow off-diagonal entries up to10. For D=3+, generated matrices can fail strict diagonal dominance and even be singular, making thesolve_exact(...).expect("diagonally-dominant A is non-singular")properties flaky.Proposed fix
- // `D + 5` upper-bounds (D · 5 + 1) once D ≤ 5 entries of magnitude ≤ 5; - // shift the supplied diagonal by this amount while preserving its sign. - let shift = f64::from(u8::try_from(D).unwrap_or(u8::MAX)).mul_add(5.0, 1.0); + // Off-diagonal entries come from `small_int_f64()` and have magnitude ≤ 10. + // Use a shift strictly larger than any row's off-diagonal sum. + let shift = f64::from(u32::try_from(D).expect("proptest dimensions fit in u32")) + .mul_add(10.0, 1.0);Also applies to: 54-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proptest_exact.rs` around lines 33 - 35, The diagonal-dominance shift currently uses D * 5 + 1 but off-diagonal entries come from small_int_f64 which yields values in [-10,10], so increase the shift to guarantee strict diagonal dominance: replace the D * 5 + 1 shift with D * 10 + 1 (or compute shift as D * max_off_diag + 1 using max_off_diag = 10) wherever the diagonal is boosted (the matrix generation/shift logic referenced around the tests that call solve_exact(...).expect("diagonally-dominant A is non-singular") — update all occurrences in the matrix-construction block(s) in tests/proptest_exact.rs, including the region covering lines ~54-77, so the diagonal boost matches the off-diagonal range).
🧹 Nitpick comments (1)
src/exact.rs (1)
2010-2164: Adversarial regression tests pin each bench group's intended behavior.The adversarial block cleanly mirrors
benches/exact.rsand the assertions are solid:
solve_exact_near_singular_3x3_integer_x0: the RHS[6 + 2^-50, 15, 24]is exactly representable in f64 (6's ulp at its exponent is 2^-50), soA · [1,1,1]materialises without rounding and the exact round-trip is a meaningful check.solve_exact_large_entries_3x3_unit_vector/det_sign_exact_large_entries_3x3_positive: the fast-filter fall-through is explicitly asserted (!a.det_direct().is_some_and(f64::is_finite)), making the intent of the test unambiguous and exercising the Bareiss cold path.- Hilbert positivity + residual tests cover D=2..=5 via macros (past D=2 gap addressed); the alternating-sign RHS in
gen_solve_exact_hilbert_residual_testsis a nice touch to avoid accidental structural cancellation.bigrational_matvectakes borrowed inputs and is test-only, consistent with the rest of the helper layout.One minor optional nit:
assert!(a.det_exact().is_ok())at Line 2089 is a very weak check — if you want it to earn its keep, you could alternatively asserta.det_exact().unwrap().is_positive()(sinceSignedis already in scope under the exact feature) to cross-validate the sign the Bareiss path just returned. Entirely optional.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" — the Hilbert macros satisfy this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exact.rs` around lines 2010 - 2164, The test det_sign_exact_large_entries_3x3_positive currently asserts a.det_exact().is_ok(), which is weak; change that to assert the exact determinant is positive by unwrapping and checking its sign (e.g. replace the is_ok() assertion with an assertion that a.det_exact().unwrap().is_positive()), referencing the test function det_sign_exact_large_entries_3x3_positive and the Matrix::det_exact method so the Bareiss result is cross-validated against det_sign_exact().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/proptest_exact.rs`:
- Around line 33-35: The diagonal-dominance shift currently uses D * 5 + 1 but
off-diagonal entries come from small_int_f64 which yields values in [-10,10], so
increase the shift to guarantee strict diagonal dominance: replace the D * 5 + 1
shift with D * 10 + 1 (or compute shift as D * max_off_diag + 1 using
max_off_diag = 10) wherever the diagonal is boosted (the matrix generation/shift
logic referenced around the tests that call
solve_exact(...).expect("diagonally-dominant A is non-singular") — update all
occurrences in the matrix-construction block(s) in tests/proptest_exact.rs,
including the region covering lines ~54-77, so the diagonal boost matches the
off-diagonal range).
---
Nitpick comments:
In `@src/exact.rs`:
- Around line 2010-2164: The test det_sign_exact_large_entries_3x3_positive
currently asserts a.det_exact().is_ok(), which is weak; change that to assert
the exact determinant is positive by unwrapping and checking its sign (e.g.
replace the is_ok() assertion with an assertion that
a.det_exact().unwrap().is_positive()), referencing the test function
det_sign_exact_large_entries_3x3_positive and the Matrix::det_exact method so
the Bareiss result is cross-validated against det_sign_exact().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15dcae8c-3e23-48e3-9016-d192df149ce4
📒 Files selected for processing (10)
.github/workflows/benchmarks.ymlAGENTS.mdREADME.mdbenches/exact.rsdocs/BENCHMARKING.mdscripts/bench_compare.pyscripts/tests/test_bench_compare.pysrc/exact.rssrc/lib.rstests/proptest_exact.rs
Introduce proptests to verify that the floating-point determinant sign matches the exact sign whenever the error bound is satisfied. This validates the correctness of the Shewchuk-style filter used in determinant calculations. Additionally, improve test reliability by correcting the diagonal dominance calculation to properly account for the maximum off-diagonal magnitude and refactor matrix-vector multiplication helpers to use idiomatic iterators. Refs: #80
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 224-227: The README sentence implies FromPrimitive, ToPrimitive,
and Signed are exported only via the prelude, but src/lib.rs also re-exports
FromPrimitive, ToPrimitive, and Signed at the crate root; update the AGENTS.md
wording to state that BigInt and BigRational are re-exported from the crate root
and prelude and that FromPrimitive, ToPrimitive, and Signed (from num-traits)
are re-exported both at the crate root and in the prelude so consumers can use
those trait methods without adding num-traits as a dependency.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 926410ad-700c-4685-a6e4-00091397332d
📒 Files selected for processing (10)
.github/workflows/benchmarks.ymlAGENTS.mdREADME.mdbenches/exact.rsdocs/BENCHMARKING.mdscripts/bench_compare.pyscripts/tests/test_bench_compare.pysrc/exact.rssrc/lib.rstests/proptest_exact.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Expand benchmark, unit-test, and proptest coverage of the exact-arithmetic APIs to catch tail cases that fixed well-conditioned inputs miss.
Benchmarks (benches/exact.rs):
bench_extreme_grouphelper running the same four benches (det_sign_exact,det_exact,solve_exact,solve_exact_f64) so adversarial groups are directly comparable.exact_near_singular_3x3with the two solve benches (the primary motivating use case for exact solve was previously unmeasured).exact_large_entries_3x3(entries nearf64::MAX / 2) to stress intermediate BigInt growth during Bareiss forward elimination.exact_hilbert_4x4/exact_hilbert_5x5to stress thef64_decompose → BigIntscaling path on ill-conditioned inputs.expect(...)messages to name the invariant each call relies on (e.g. "non-singular matrix with finite entries") so panics identify both where (Criterion bench name) and why.Unit tests (src/exact.rs):
solve_exact_near_singular_3x3_integer_x0— integer-x0 round-trip through the 2^-50-perturbed matrix.solve_exact_large_entries_3x3_unit_vector—A·[1,0,0] = [big,1,1]round-trip withf64::MAX/2diagonal.det_sign_exact_large_entries_3x3_positive— asserts the fast filter falls through (det_directis non-finite) anddet_exact_f64returnsOverflow { index: None }.det_sign_exact_hilbert_positive_{3,4,5}d— Hilbert is SPD, sign = 1.solve_exact_hilbert_residual_{3,4,5}d— residualA·x - bis exactly zero inBigRational, stronger than integer round-trips since Hilbert entries are non-terminating in binary.Proptests (tests/proptest_exact.rs):
solve_exact_integer_roundtrip_{2..5}d— random diagonally-dominant integerAand small-integerx0, verifysolve(A, A·x0) == x0exactly.solve_exact_residual_{2..5}d— randomA+ small-integerb, verifyA · solve(A, b) == bexactly (catches back-sub bugs on fractional solutions).det_sign_exact_agrees_with_det_exact_{2..5}d— on full (non-diagonal) small-integer matrices, assertsdet_sign_exact() == det_exact().sign()(exercises the filter/fallback boundary previously only diagonal-tested).Prelude (src/lib.rs):
BigIntalongsideBigRational(crate root + prelude).FromPrimitive,Signed,ToPrimitivefromnum-traitsin the prelude so the re-exportedBigRationalis actually usable for construction (from_f64,from_i64) and sign queries without forcing downstream users to addnum-bigint/num-rational/num-traitsto their own Cargo.toml. Additive; no public API breaks.Tooling (scripts/bench_compare.py + test_bench_compare.py):
EXACT_GROUPS._group_headingwith human-readable titles ("Large entries 3x3", "Hilbert 4x4", "Hilbert 5x5").Test results (
just ci):-D warnings): cleanCloses #80
Summary by CodeRabbit
New Features
Tests
Benchmarks
Documentation
Chores