feat!(api): make Matrix and Vector finite by construction#147
Merged
Conversation
- Parse raw matrix and vector storage through Matrix::try_from_rows and Vector::try_new, with private storage carrying the finite-entry invariant through downstream kernels. - Rename LU and LDLT solves to solve, and remove compatibility aliases that weaken the finite-proof API model. - Add Semgrep guardrails for the finite API contract and public-documentation unwrap/expect usage. - Update benchmarks, examples, docs, and tooling pins for the new API and current Markdown/spelling lint behavior. BREAKING CHANGE: Matrix::from_rows is no longer a public raw constructor; use Matrix::try_from_rows for raw row-major input. BREAKING CHANGE: Vector::new is no longer a public raw constructor; use Vector::try_new for raw vector input. BREAKING CHANGE: Lu::solve_vec and Ldlt::solve_vec are removed; use solve. BREAKING CHANGE: DEFAULT_PIVOT_TOL is removed; use DEFAULT_SINGULAR_TOL. Closes #137
Contributor
📝 WalkthroughWalkthroughThis PR consolidates the finite-by-construction invariant introduced in v0.4.2 by removing the public ChangesFinite-by-Construction Invariant and Solve API Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 #147 +/- ##
==========================================
- Coverage 99.61% 99.58% -0.03%
==========================================
Files 5 5
Lines 2846 2664 -182
==========================================
- Hits 2835 2653 -182
Misses 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
12-573:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not edit
CHANGELOG.mddirectly; regenerate it via the release tooling.This file has direct manual edits, which breaks the repo’s changelog generation contract and risks drift from commit-derived release notes. Please move these edits into commits/messages as needed and regenerate with
just changelog(orjust changelog-unreleased <version>for prepend flow).
As per coding guidelines: “CHANGELOG.md: Never editCHANGELOG.mddirectly — it's auto-generated from git commits; usejust changelogto regenerate orjust changelog-unreleased <version>to prepend unreleased changes.”🤖 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 `@CHANGELOG.md` around lines 12 - 573, The PR modified CHANGELOG.md directly, which violates the repo contract; revert any manual edits to CHANGELOG.md, move the intended release notes into proper commit messages or commits, then regenerate the changelog using the release tooling (run just changelog for standard regeneration or just changelog-unreleased <version> to prepend an unreleased entry). Ensure the final change is produced by the tooling rather than hand-editing CHANGELOG.md so the file remains commit-derived and consistent with the release pipeline.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 `@tests/semgrep/src/project_rules/readme_doctest_mirrors.rs`:
- Around line 5-35: The file declares the module name readme_doctests three
times causing E0428; rename each module to a unique identifier (e.g.,
readme_doctests_unwrap, readme_doctests_expect, readme_doctests_result) so that
the three modules (the ones containing the tests readme_mirror_uses_unwrap,
readme_mirror_uses_expect, and
readme_mirror_uses_result/ordinary_internal_tests_may_use_unwrap) no longer
collide; ensure any internal references to the old module name are updated
accordingly.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 12-573: The PR modified CHANGELOG.md directly, which violates the
repo contract; revert any manual edits to CHANGELOG.md, move the intended
release notes into proper commit messages or commits, then regenerate the
changelog using the release tooling (run just changelog for standard
regeneration or just changelog-unreleased <version> to prepend an unreleased
entry). Ensure the final change is produced by the tooling rather than
hand-editing CHANGELOG.md so the file remains commit-derived and consistent with
the release pipeline.
🪄 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: f565d7e5-06fe-4522-827e-ddd9ebdd272b
📒 Files selected for processing (25)
.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdREADME.mdbenches/vs_linalg.rsdocs/roadmap.mdexamples/det_5x5.rsexamples/exact_solve_3x3.rsexamples/ldlt_solve_3x3.rsexamples/solve_5x5.rsjustfilescripts/postprocess_changelog.pyscripts/tests/test_postprocess_changelog.pysemgrep.yamlsrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/vector.rstests/proptest_factorizations.rstests/proptest_matrix.rstests/semgrep/src/project_rules/finite_api_contract.rstests/semgrep/src/project_rules/readme_doctest_mirrors.rstests/vs_linalg_inputs.rs
Comment on lines
+5
to
+35
| mod readme_doctests { | ||
| #[test] | ||
| fn readme_mirror_uses_unwrap() { | ||
| let _ = Some(1_u32).unwrap(); | ||
| } | ||
| } | ||
|
|
||
| // ruleid: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors | ||
| mod readme_doctests { | ||
| #[test] | ||
| fn readme_mirror_uses_expect() { | ||
| let _ = Ok::<u32, &'static str>(1).expect("README mirrors should use ?"); | ||
| } | ||
| } | ||
|
|
||
| // ok: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors | ||
| mod tests { | ||
| #[test] | ||
| fn ordinary_internal_tests_may_use_unwrap() { | ||
| let _ = Some(1_u32).unwrap(); | ||
| } | ||
| } | ||
|
|
||
| // ok: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors | ||
| mod readme_doctests { | ||
| #[test] | ||
| fn readme_mirror_uses_result() -> Result<(), &'static str> { | ||
| let _ = Ok::<u32, &'static str>(1)?; | ||
| Ok(()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix duplicate module names.
Rust does not permit multiple modules with the same name in the same scope. This file defines mod readme_doctests three times (lines 5, 13, 29), which will cause compilation error E0428.
🔧 Proposed fix
Rename the modules to have unique names:
-// ruleid: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors
-mod readme_doctests {
+// ruleid: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors
+mod readme_doctests_unwrap {
#[test]
fn readme_mirror_uses_unwrap() {
let _ = Some(1_u32).unwrap();
}
}
// ruleid: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors
-mod readme_doctests {
+mod readme_doctests_expect {
#[test]
fn readme_mirror_uses_expect() {
let _ = Ok::<u32, &'static str>(1).expect("README mirrors should use ?");
}
}
// ok: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors
mod tests {
#[test]
fn ordinary_internal_tests_may_use_unwrap() {
let _ = Some(1_u32).unwrap();
}
}
// ok: la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors
-mod readme_doctests {
+mod readme_doctests_result {
#[test]
fn readme_mirror_uses_result() -> Result<(), &'static str> {
let _ = Ok::<u32, &'static str>(1)?;
Ok(())
}
}🧰 Tools
🪛 OpenGrep (1.22.0)
[WARNING] 5-8: Use fallible flow instead of unwrap() or expect() in src/lib.rs README doctest mirrors.
(la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors)
[WARNING] 13-16: Use fallible flow instead of unwrap() or expect() in src/lib.rs README doctest mirrors.
(la-stack.rust.no-unwrap-expect-in-readme-doctest-mirrors)
🤖 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 `@tests/semgrep/src/project_rules/readme_doctest_mirrors.rs` around lines 5 -
35, The file declares the module name readme_doctests three times causing E0428;
rename each module to a unique identifier (e.g., readme_doctests_unwrap,
readme_doctests_expect, readme_doctests_result) so that the three modules (the
ones containing the tests readme_mirror_uses_unwrap, readme_mirror_uses_expect,
and readme_mirror_uses_result/ordinary_internal_tests_may_use_unwrap) no longer
collide; ensure any internal references to the old module name are updated
accordingly.
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.
BREAKING CHANGE: Matrix::from_rows is no longer a public raw constructor; use Matrix::try_from_rows for raw row-major input.
BREAKING CHANGE: Vector::new is no longer a public raw constructor; use Vector::try_new for raw vector input.
BREAKING CHANGE: Lu::solve_vec and Ldlt::solve_vec are removed; use solve.
BREAKING CHANGE: DEFAULT_PIVOT_TOL is removed; use DEFAULT_SINGULAR_TOL.
Closes #137
Summary by CodeRabbit
Breaking Changes
solve_vectosolveDEFAULT_PIVOT_TOLconstant; useDEFAULT_SINGULAR_TOLinsteadDocumentation
solvemethod nameChores