perf: integer-only Bareiss determinant via BigInt (#64)#73
Merged
acgetchell merged 2 commits intomainfrom Apr 11, 2026
Merged
Conversation
Replace the BigRational Bareiss determinant with a pure BigInt path: all f64 entries are decomposed into mantissa × 2^exponent, scaled to a common integer base, and eliminated without any rational arithmetic. The result is reconstructed as BigRational only at the end. - Add f64_decompose helper (extracted from f64_to_bigrational) - Add bareiss_det_int: integer-only Bareiss returning (BigInt, i32) - Add bigint_exp_to_bigrational: reconstruction with trailing-zero reduction (no full GCD needed) - Refactor bareiss_det as thin wrapper over bareiss_det_int - Optimize det_sign_exact to read sign from BigInt directly (skip BigRational reconstruction entirely) - Import std::array::from_fn for shorter call sites - Replace clippy cast suppression with exact try_from conversions - Update AGENTS.md with non-interactive gh CLI patterns - 24 new tests (256 total): f64_decompose, bareiss_det_int (D=0–5, fractional, all-zeros, sign), bigint_exp_to_bigrational (positive/ negative exp, reduction, negative values) Performance (vs pre-bigint baseline on Apple M4 Max): det_exact: 16x (D=2) → 39x (D=5) faster det_exact_f64: 10x (D=2) → 38x (D=5) faster det_sign_exact: 40x at D=5 (bypasses BigRational entirely) Near-singular: 18x faster Co-Authored-By: Oz <oz-agent@warp.dev>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR replaces the BigRational-backed Bareiss determinant path with an integer-only Bareiss pipeline using f64 decomposition to mantissa/exponent, updates det_sign_exact to use the integer determinant sign, modernizes some array construction, expands tests, and adds GitHub CLI guidance in AGENTS.md (plus a small REFERENCES.md clarification). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 #73 +/- ##
==========================================
+ Coverage 88.75% 90.26% +1.51%
==========================================
Files 5 5
Lines 409 452 +43
==========================================
+ Hits 363 408 +45
+ Misses 46 44 -2
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/exact.rs (1)
189-294:⚠️ Potential issue | 🟡 MinorUpdate the public rustdoc to match the new BigInt determinant path.
The implementation now routes exact determinant/sign work through
bareiss_det_int, but the module-level docs anddet_sign_exactrustdoc still describe aBigRationalBareiss core. That doc drift will mislead readers about both the algorithm and where the speedup comes from.Also applies to: 548-555
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exact.rs` around lines 189 - 294, Update the rustdoc to reflect that exact determinant/sign work now uses an integer-only Bareiss core via bareiss_det_int returning (BigInt, exponent) rather than a BigRational Bareiss; update module-level docs and the det_sign_exact function doc to mention bareiss_det_int and bigint_exp_to_bigrational (and that bareiss_det simply wraps these), describe the integer matrix construction and exponent accounting (mantissa shifting to form BigInt matrix and total_exp = D * e_min), and note this is the source of the performance improvement instead of a BigRational-based Bareiss implementation.
🧹 Nitpick comments (1)
AGENTS.md (1)
198-198: Makegh issue listconsistent with the documented--json | catpattern.Line 198 uses plain
gh issue listwhile line 197 and the guidance above (line 179) recommend always using--json | catto avoid pager and scope errors. Update for consistency:Suggested change
-- **List**: `gh issue list` (add `--label enhancement`, `--milestone v0.4.0`, etc. to filter) +- **List**: `gh issue list --json number,title,labels,milestone --jq '.[]' | cat` (add `--label enhancement`, `--milestone v0.4.0`, etc. to filter)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 198, The example uses plain `gh issue list` which is inconsistent with the documented pattern; update the example so `gh issue list` includes the `--json ... | cat` usage (e.g., `gh issue list --label enhancement --milestone v0.4.0 --json ... | cat`) to match the guidance referenced earlier and avoid pager/scope issues; locate the `gh issue list` example and replace it with the `--json | cat` variant while preserving the suggested flags such as `--label` and `--milestone`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/exact.rs`:
- Around line 189-294: Update the rustdoc to reflect that exact determinant/sign
work now uses an integer-only Bareiss core via bareiss_det_int returning
(BigInt, exponent) rather than a BigRational Bareiss; update module-level docs
and the det_sign_exact function doc to mention bareiss_det_int and
bigint_exp_to_bigrational (and that bareiss_det simply wraps these), describe
the integer matrix construction and exponent accounting (mantissa shifting to
form BigInt matrix and total_exp = D * e_min), and note this is the source of
the performance improvement instead of a BigRational-based Bareiss
implementation.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 198: The example uses plain `gh issue list` which is inconsistent with
the documented pattern; update the example so `gh issue list` includes the
`--json ... | cat` usage (e.g., `gh issue list --label enhancement --milestone
v0.4.0 --json ... | cat`) to match the guidance referenced earlier and avoid
pager/scope issues; locate the `gh issue list` example and replace it with the
`--json | cat` variant while preserving the suggested flags such as `--label`
and `--milestone`.
Update architecture descriptions in AGENTS.md and REFERENCES.md to reflect the move from BigRational to integer-only BigInt arithmetic. Add unit tests for bareiss_det_int covering negative inputs and pivot swapping. Refine gh CLI patterns for automated issue listing. Refs: #64
This was referenced Apr 20, 2026
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.
Replace the BigRational Bareiss determinant with a pure BigInt path: all f64 entries are decomposed into mantissa × 2^exponent, scaled to a common integer base, and eliminated without any rational arithmetic. The result is reconstructed as BigRational only at the end.
Performance (vs pre-bigint baseline on Apple M4 Max):
det_exact: 16x (D=2) → 39x (D=5) faster
det_exact_f64: 10x (D=2) → 38x (D=5) faster
det_sign_exact: 40x at D=5 (bypasses BigRational entirely)
Near-singular: 18x faster
Summary by CodeRabbit