feat(api)!: make numerical invariants and errors explicit#178
Merged
Conversation
- add structured error reasons, locations, origins, and factorization context - make determinant, exact-conversion, LU, and LDLT paths range-safe and mathematically explicit - validate benchmark inputs independently and require reproducible, provenance-backed performance evidence - centralize tool versions, adopt nextest profiles, and replace Codacy with repository-owned checks and SARIF reporting BREAKING CHANGE: Rename `Tolerance::new` to `Tolerance::try_new` and `Matrix::get_checked` to `Matrix::try_get`; remove `Matrix::set_checked` in favor of `Matrix::set`. `Vector::dot` and `Vector::norm2_sq` now borrow their operands. `det_sign_exact` is now infallible and returns `DeterminantSign` instead of `Result<i8, LaError>`. `LaError` variants now use typed reason, location, and origin fields and require `..` in downstream matches. LDLT now requires exact symmetry, determinant error bounds may be unavailable under gradual underflow, and `ERR_COEFF_2`, `ERR_COEFF_3`, and `ERR_COEFF_4` are no longer exported by the prelude.
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 ignored due to path filters (1)
📒 Files selected for processing (44)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (27)
📝 WalkthroughWalkthroughThis PR refactors numerical APIs and typed errors, exact arithmetic and factorization kernels, deterministic benchmark/reporting workflows, CI and repository tooling, validation tests, and contributor documentation. ChangesNumerical API and implementation
Benchmark and reporting workflows
Automation and repository guidance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 98.13% 97.75% -0.39%
==========================================
Files 7 8 +1
Lines 3323 4624 +1301
==========================================
+ Hits 3261 4520 +1259
- Misses 62 104 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/bench_compare.py (1)
1119-1133: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical: Python 2 exception syntax — module fails to parse.
except ExecutableNotFoundError, subprocess.CalledProcessError:is invalid Python 3 syntax (raisesSyntaxError). This breaks importing the entire module, which cascades tomain(), all tests inscripts/tests/test_bench_compare.py, and any script importingbench_compare(e.g.archive_performance.py).🐛 Proposed fix
try: result = run_git_command(["--no-pager", "rev-parse", "--short", "HEAD"], cwd=root) short_hash = result.stdout.strip() - except ExecutableNotFoundError, subprocess.CalledProcessError: + except (ExecutableNotFoundError, subprocess.CalledProcessError): pass try: result = run_git_command(["--no-pager", "rev-parse", "--abbrev-ref", "HEAD"], cwd=root) branch = result.stdout.strip() - except ExecutableNotFoundError, subprocess.CalledProcessError: + except (ExecutableNotFoundError, subprocess.CalledProcessError): pass🤖 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 `@scripts/bench_compare.py` around lines 1119 - 1133, Fix the invalid exception syntax in _get_git_info by using Python 3 tuple syntax to catch both ExecutableNotFoundError and subprocess.CalledProcessError in each try block, preserving the existing fallback behavior.
🧹 Nitpick comments (3)
scripts/criterion_dim_plot.py (1)
872-890: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInconsistent type for
criterion.benchmark_commandprovenance field.
benchmark_commandis alist[str]whenmeasurement_recordedis true but the bare string"unavailable"otherwise. Consumers of this JSON (including the README publication test that indexes[:3]) would break if they don't special-case the exploratory branch. Consider using a consistently-typed value (e.g.None, or always a list with a sentinel) for a stable schema.🤖 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 `@scripts/criterion_dim_plot.py` around lines 872 - 890, Make criterion.benchmark_command type-consistent in the returned provenance object: update the conditional in the result-building function to use a single stable representation, preferably None when measurement_recorded is false and the existing list when true. Update any consumers or schema expectations, including README publication handling, to support the chosen representation without indexing a string sentinel.src/error.rs (1)
532-548: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
invalid_tolerancecan mislabel a valid tolerance if misused externally.This
pub const fninfersInvalidToleranceReasonsolely fromvalue.is_finite(), trusting that the caller already confirmedvalueviolates the invariant. If an external caller invokesLaError::invalid_tolerance(5.0)directly (a valid, non-negative finite value), it silently constructsInvalidTolerance { value: 5.0, reason: Negative }— a logically wrong, misleading error. The doc comment flags this as a precondition, but nothing enforces it for a public API.Consider either accepting an explicit
InvalidToleranceReasonfrom the caller (mirroring howsingular_numericaltakes explicit typed fields) ordebug_assert!-checking the precondition to catch misuse in tests/debug builds.♻️ Proposed direction
- pub const fn invalid_tolerance(value: f64) -> Self { - let reason = if value.is_finite() { - InvalidToleranceReason::Negative - } else { - InvalidToleranceReason::NotFinite - }; - Self::InvalidTolerance { value, reason } - } + pub const fn invalid_tolerance(value: f64) -> Self { + debug_assert!( + !value.is_finite() || value < 0.0, + "invalid_tolerance called with a value that satisfies the tolerance invariant" + ); + let reason = if value.is_finite() { + InvalidToleranceReason::Negative + } else { + InvalidToleranceReason::NotFinite + }; + Self::InvalidTolerance { value, reason } + }🤖 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 `@src/error.rs` around lines 532 - 548, Harden the public constructor invalid_tolerance against valid inputs being mislabeled: either change it to accept an explicit InvalidToleranceReason, or retain inference while adding a debug_assert! that value is non-finite or negative before constructing InvalidTolerance. Update all callers and documentation to match the chosen contract, preserving the NotFinite precedence for negative infinity.scripts/bench_compare.py (1)
916-919: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
assessmentparameter in_format_pct.The parameter is immediately discarded (
del assessment) and never affects output. Consider dropping it from the signature and call sites if no future use is planned, or keep as-is if intentionally reserved for future formatting.🤖 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 `@scripts/bench_compare.py` around lines 916 - 919, The _format_pct function accepts an unused assessment parameter and immediately discards it; remove assessment from the function signature and update every call site accordingly, unless it is intentionally retained for planned formatting behavior.
🤖 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 `@AGENTS.md`:
- Around line 133-139: Repair the bullet list under “Testing mirrors the
principles” by keeping the “Unit tests cover known values, error paths, and
dimension-generic correctness across D=2..=5 (see Dimension Coverage below)”
text as one bullet, then placing “Error-path tests match the exact variant...”
as a separate bullet.
In `@scripts/criterion_dim_plot.py`:
- Around line 739-745: Fix the invalid Python exception syntax by parenthesizing
the exception tuples: update the except clauses in _git_value,
_git_status_metadata, and _rustc_version to use `except
(ExecutableNotFoundError, subprocess.CalledProcessError):`.
- Around line 670-687: Pin the rollback temporary directory to the benchmark
root in _run_publication_benchmarks by passing dir=root to
tempfile.TemporaryDirectory, matching _stage_and_publish_outputs so staged files
remain on the same filesystem as target/criterion before Path.replace restores
them.
In `@scripts/subprocess_utils.py`:
- Line 184: Update both exception handlers in the relevant subprocess utility
functions to use Python 3 tuple syntax: replace `except ExecutableNotFoundError,
subprocess.CalledProcessError:` with `except (ExecutableNotFoundError,
subprocess.CalledProcessError):`.
---
Outside diff comments:
In `@scripts/bench_compare.py`:
- Around line 1119-1133: Fix the invalid exception syntax in _get_git_info by
using Python 3 tuple syntax to catch both ExecutableNotFoundError and
subprocess.CalledProcessError in each try block, preserving the existing
fallback behavior.
---
Nitpick comments:
In `@scripts/bench_compare.py`:
- Around line 916-919: The _format_pct function accepts an unused assessment
parameter and immediately discards it; remove assessment from the function
signature and update every call site accordingly, unless it is intentionally
retained for planned formatting behavior.
In `@scripts/criterion_dim_plot.py`:
- Around line 872-890: Make criterion.benchmark_command type-consistent in the
returned provenance object: update the conditional in the result-building
function to use a single stable representation, preferably None when
measurement_recorded is false and the existing list when true. Update any
consumers or schema expectations, including README publication handling, to
support the chosen representation without indexing a string sentinel.
In `@src/error.rs`:
- Around line 532-548: Harden the public constructor invalid_tolerance against
valid inputs being mislabeled: either change it to accept an explicit
InvalidToleranceReason, or retain inference while adding a debug_assert! that
value is non-finite or negative before constructing InvalidTolerance. Update all
callers and documentation to match the chosen contract, preserving the NotFinite
precedence for negative infinity.
🪄 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: c9a226d6-c2c9-4658-b356-2ee85d1ac055
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.codacy.yml.config/nextest.toml.github/workflows/audit.yml.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/codacy.yml.github/workflows/codecov.yml.github/workflows/codeql.yml.github/workflows/release-benchmarks.yml.github/workflows/rust-clippy.yml.github/workflows/semgrep-sarif.yml.markdownlint.json.python-version.yamllintAGENTS.mdCONTRIBUTING.mdCargo.tomlREADME.mdREFERENCES.mdSECURITY.mdbenches/common/exact.rsbenches/common/vs_linalg.rsbenches/exact.rsbenches/vs_linalg.rsclippy.tomldocs/BENCHMARKING.mddocs/COVERAGE.mddocs/PERFORMANCE.mddocs/RELEASING.mddocs/assets/bench/vs_linalg_lu_solve_median.provenance.jsondocs/roadmap.mddprint.jsonexamples/const_det_4x4.rsexamples/det_5x5.rsexamples/exact_det_3x3.rsexamples/exact_sign_3x3.rsexamples/exact_solve_3x3.rsexamples/ldlt_solve_3x3.rsexamples/solve_5x5.rsjustfilepyproject.tomlrust-toolchain.tomlscripts/README.mdscripts/archive_changelog.pyscripts/archive_performance.pyscripts/bench_compare.pyscripts/check_docs_version_sync.pyscripts/check_semgrep_fixtures.pyscripts/criterion_dim_plot.pyscripts/postprocess_changelog.pyscripts/subprocess_utils.pyscripts/tests/__init__.pyscripts/tests/test_archive_performance.pyscripts/tests/test_bench_compare.pyscripts/tests/test_check_docs_version_sync.pyscripts/tests/test_check_semgrep_fixtures.pyscripts/tests/test_criterion_dim_plot.pyscripts/tests/test_subprocess_utils.pyscripts/tests/test_tag_release.pysemgrep.yamlsrc/error.rssrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/scaled_product.rssrc/tolerance.rssrc/vector.rstests/exact_bench_config.rstests/exact_conversion_boundaries.rstests/prelude_exports.rstests/proptest_exact.rstests/proptest_factorizations.rstests/proptest_matrix.rstests/proptest_vector.rstests/regressions.rstests/scaled_product_determinants.rstests/semgrep/.github/workflows/action_policy.ymltests/semgrep/docs/public_examples.mdtests/semgrep/scripts/tests/python_exceptions.pytests/semgrep/src/project_rules/finite_api_contract.rstests/semgrep/src/project_rules/portable_policy.rstests/vs_linalg_inputs.rsty.toml
💤 Files with no reviewable changes (4)
- .codacy.yml
- .github/workflows/codacy.yml
- ty.toml
- .markdownlint.json
Comment on lines
133
to
139
| ### Testing mirrors the principles | ||
|
|
||
| - Unit tests cover known values, error paths, and dimension-generic | ||
| - Error-path tests match the exact variant, typed reason/origin/location, and | ||
| structured fields; do not replace an unexpected error with a numeric sentinel | ||
| or assert only `is_err()`. | ||
| correctness across D=2..=5 (see **Dimension Coverage** below). |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Bullet list corrupted — sentence spliced into the wrong bullet.
The new "Error-path tests..." bullet was inserted between the "Unit tests..." bullet and its own wrapped continuation line. As rendered, the first bullet now dead-ends at "dimension-generic" and the orphaned "correctness across D=2..=5 (see Dimension Coverage below)." sentence is attached to the unrelated second bullet instead.
📝 Proposed fix
-- Unit tests cover known values, error paths, and dimension-generic
-- Error-path tests match the exact variant, typed reason/origin/location, and
- structured fields; do not replace an unexpected error with a numeric sentinel
- or assert only `is_err()`.
- correctness across D=2..=5 (see **Dimension Coverage** below).
+- Unit tests cover known values, error paths, and dimension-generic
+ correctness across D=2..=5 (see **Dimension Coverage** below).
+- Error-path tests match the exact variant, typed reason/origin/location, and
+ structured fields; do not replace an unexpected error with a numeric sentinel
+ or assert only `is_err()`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Testing mirrors the principles | |
| - Unit tests cover known values, error paths, and dimension-generic | |
| - Error-path tests match the exact variant, typed reason/origin/location, and | |
| structured fields; do not replace an unexpected error with a numeric sentinel | |
| or assert only `is_err()`. | |
| correctness across D=2..=5 (see **Dimension Coverage** below). | |
| ### Testing mirrors the principles | |
| - Unit tests cover known values, error paths, and dimension-generic | |
| correctness across D=2..=5 (see **Dimension Coverage** below). | |
| - Error-path tests match the exact variant, typed reason/origin/location, and | |
| structured fields; do not replace an unexpected error with a numeric sentinel | |
| or assert only `is_err()`. |
🤖 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 `@AGENTS.md` around lines 133 - 139, Repair the bullet list under “Testing
mirrors the principles” by keeping the “Unit tests cover known values, error
paths, and dimension-generic correctness across D=2..=5 (see Dimension Coverage
below)” text as one bullet, then placing “Error-path tests match the exact
variant...” as a separate bullet.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benches/common/exact.rs (1)
22-49: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMark
ExactBenchConfigErrorandUnorderedRangeas#[non_exhaustive]
ExactBenchConfigErrorandExactBenchConfigError::UnorderedRangeshould be#[non_exhaustive]so this public configuration error can evolve without breaking matches.I16Rangeis already#[must_use].🤖 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 `@benches/common/exact.rs` around lines 22 - 49, Mark the public ExactBenchConfigError enum and its UnorderedRange variant with #[non_exhaustive]. Leave I16Range unchanged, as it already has #[must_use].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 `@benches/common/vs_linalg.rs`:
- Around line 127-140: Restrict make_ill_conditioned_matrix_rows to its
supported dimension D=8, updating its signature and documentation so callers
cannot request dimensions where diagonal scaling underflows; adjust any call
sites or tests accordingly. Alternatively, retain genericity only by returning a
typed failure for unsupported dimensions, while ensuring the fixture never
produces zero diagonal entries or violates positive-definiteness.
In `@Cargo.toml`:
- Around line 14-33: Update the Cargo.toml package include whitelist to include
the checked-in docs/assets/bench/*.provenance.json provenance artifact,
preserving the existing asset patterns and formatting the TOML with the
repository’s just toml-* commands.
In `@scripts/archive_changelog.py`:
- Around line 315-338: Update _archive_dir_link_prefix to fail with a clear
exception when os.path.relpath() raises ValueError, rather than falling back to
archive_dir.as_posix(); ensure archive_changelog() propagates or handles this
failure before rewriting CHANGELOG.md. Add a regression test simulating
cross-volume paths and verify no changelog rewrite occurs and no absolute
filesystem path is emitted.
In `@scripts/archive_performance.py`:
- Around line 1038-1042: Update the fallback branch in the current benchmark
command selection to choose the cargo benchmark and feature arguments based on
config.suite, matching the suite-specific split implemented by
_baseline_tool_args; ensure vs_linalg runs its corresponding benchmark instead
of always running exact.
In `@scripts/bench_compare.py`:
- Around line 1216-1290: Update _provenance_markdown to render correctness-gate
wording based on the provenance mode: retain “shared current fixture harness”
only for current/shared-harness validation, and use wording that accurately
describes historical-assets validation without asserting an unverified harness.
Add or update tests covering historical-assets mode with unavailable measurement
metadata, verifying the published provenance contains only mode-appropriate
wording.
In `@scripts/check_docs_version_sync.py`:
- Around line 260-263: Update _README_TAG_LINK_RE so prerelease and build
metadata suffixes are independently optional, allowing versions such as
v1.2.3-rc.1+build.7 to match; add or update the relevant tests to verify both
combined suffixes and existing version formats.
In `@scripts/criterion_dim_plot.py`:
- Around line 689-702: Update _run_publication_command to also catch
ExecutableNotFoundError and subprocess.TimeoutExpired from run_safe_command,
converting both into RuntimeError with the command and relevant failure details
preserved. Keep CalledProcessError handling intact so every publication-command
failure reaches main’s staging rollback path.
- Around line 743-744: Replace each invalid multi-exception handler in the
relevant functions with Python 3 parenthesized exception tuple syntax, covering
ExecutableNotFoundError and subprocess.CalledProcessError at all three
occurrences.
In `@scripts/subprocess_utils.py`:
- Line 184: Update the exception handlers in the relevant subprocess utility
function to use Python 3 tuple syntax, changing both `except
ExecutableNotFoundError, subprocess.CalledProcessError` clauses to catch
`(ExecutableNotFoundError, subprocess.CalledProcessError)`, so the module
imports cleanly.
---
Outside diff comments:
In `@benches/common/exact.rs`:
- Around line 22-49: Mark the public ExactBenchConfigError enum and its
UnorderedRange variant with #[non_exhaustive]. Leave I16Range unchanged, as it
already has #[must_use].
🪄 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: c9a226d6-c2c9-4658-b356-2ee85d1ac055
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.codacy.yml.config/nextest.toml.github/workflows/audit.yml.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/codacy.yml.github/workflows/codecov.yml.github/workflows/codeql.yml.github/workflows/release-benchmarks.yml.github/workflows/rust-clippy.yml.github/workflows/semgrep-sarif.yml.markdownlint.json.python-version.yamllintAGENTS.mdCONTRIBUTING.mdCargo.tomlREADME.mdREFERENCES.mdSECURITY.mdbenches/common/exact.rsbenches/common/vs_linalg.rsbenches/exact.rsbenches/vs_linalg.rsclippy.tomldocs/BENCHMARKING.mddocs/COVERAGE.mddocs/PERFORMANCE.mddocs/RELEASING.mddocs/assets/bench/vs_linalg_lu_solve_median.provenance.jsondocs/roadmap.mddprint.jsonexamples/const_det_4x4.rsexamples/det_5x5.rsexamples/exact_det_3x3.rsexamples/exact_sign_3x3.rsexamples/exact_solve_3x3.rsexamples/ldlt_solve_3x3.rsexamples/solve_5x5.rsjustfilepyproject.tomlrust-toolchain.tomlscripts/README.mdscripts/archive_changelog.pyscripts/archive_performance.pyscripts/bench_compare.pyscripts/check_docs_version_sync.pyscripts/check_semgrep_fixtures.pyscripts/criterion_dim_plot.pyscripts/postprocess_changelog.pyscripts/subprocess_utils.pyscripts/tests/__init__.pyscripts/tests/test_archive_performance.pyscripts/tests/test_bench_compare.pyscripts/tests/test_check_docs_version_sync.pyscripts/tests/test_check_semgrep_fixtures.pyscripts/tests/test_criterion_dim_plot.pyscripts/tests/test_subprocess_utils.pyscripts/tests/test_tag_release.pysemgrep.yamlsrc/error.rssrc/exact.rssrc/ldlt.rssrc/lib.rssrc/lu.rssrc/matrix.rssrc/scaled_product.rssrc/tolerance.rssrc/vector.rstests/exact_bench_config.rstests/exact_conversion_boundaries.rstests/prelude_exports.rstests/proptest_exact.rstests/proptest_factorizations.rstests/proptest_matrix.rstests/proptest_vector.rstests/regressions.rstests/scaled_product_determinants.rstests/semgrep/.github/workflows/action_policy.ymltests/semgrep/docs/public_examples.mdtests/semgrep/scripts/tests/python_exceptions.pytests/semgrep/src/project_rules/finite_api_contract.rstests/semgrep/src/project_rules/portable_policy.rstests/vs_linalg_inputs.rsty.toml
💤 Files with no reviewable changes (4)
- .codacy.yml
- .markdownlint.json
- .github/workflows/codacy.yml
- ty.toml
- Adapt the shared benchmark harness across v0.4.3 API differences without changing measured operations - Exclude invalid balanced-range baselines while requiring current samples and reporting unavailable comparisons - Preserve benchmark provenance, suite-specific fallback commands, and publication rollback guarantees - Harden Windows Git input, changelog links, and version-reference parsing across platforms
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)
benches/common/vs_linalg.rs (1)
59-112: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winMove
det(P)out of the timed path
faer_det_from_partial_piv_lustill computesfaer_perm_sign(lu.P())inside the benchmarkedbencher.iterclosure, so this parity work is part of the measuredfaer_det_from_luresult. Precompute the sign before timing, or benchmark parity separately, to keep the comparison focused on determinant work.🤖 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 `@benches/common/vs_linalg.rs` around lines 59 - 112, Move the faer_perm_sign(lu.P()) computation out of the bencher.iter closure in faer_det_from_partial_piv_lu by calculating it before timing and reusing the result inside the closure. Keep the benchmark’s timed path focused on determinant computation while preserving the existing determinant result.Source: Learnings
🤖 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 `@scripts/tests/test_archive_changelog.py`:
- Around line 465-466: Update the assertions in the archive changelog test to
verify that both archive_dir’s native string representation and its POSIX
representation are absent from root and str(exc_info.value). Preserve the
existing leak checks while covering Windows backslash paths as well as
slash-separated paths.
---
Outside diff comments:
In `@benches/common/vs_linalg.rs`:
- Around line 59-112: Move the faer_perm_sign(lu.P()) computation out of the
bencher.iter closure in faer_det_from_partial_piv_lu by calculating it before
timing and reusing the result inside the closure. Keep the benchmark’s timed
path focused on determinant computation while preserving the existing
determinant result.
🪄 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: a4f5470d-2e47-40b9-b4c6-6dfb1358b843
📒 Files selected for processing (22)
Cargo.tomlbenches/common/exact.rsbenches/common/vs_linalg.rsbenches/vs_linalg.rsdocs/BENCHMARKING.mdscripts/archive_changelog.pyscripts/archive_performance.pyscripts/bench_compare.pyscripts/check_docs_version_sync.pyscripts/criterion_dim_plot.pyscripts/subprocess_utils.pyscripts/tests/test_archive_changelog.pyscripts/tests/test_archive_performance.pyscripts/tests/test_bench_compare.pyscripts/tests/test_check_docs_version_sync.pyscripts/tests/test_criterion_dim_plot.pyscripts/tests/test_subprocess_utils.pysrc/error.rssrc/ldlt.rssrc/matrix.rssrc/scaled_product.rstests/vs_linalg_inputs.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- scripts/archive_changelog.py
- Cargo.toml
- scripts/tests/test_check_docs_version_sync.py
- benches/vs_linalg.rs
- scripts/check_docs_version_sync.py
- benches/common/exact.rs
- tests/vs_linalg_inputs.rs
- src/error.rs
- scripts/tests/test_criterion_dim_plot.py
- src/ldlt.rs
- scripts/tests/test_archive_performance.py
- scripts/criterion_dim_plot.py
- scripts/archive_performance.py
- docs/BENCHMARKING.md
- scripts/bench_compare.py
- src/matrix.rs
- add `DeterminantWithErrorBound` for paired determinant estimates and certified bounds - scale exact systems independently and round exact values directly to IEEE-754 - fail benchmark publication closed on invalid samples or mismatched provenance - make release and changelog tooling transactional, path-safe, and Windows-portable - align benchmark CI with pinned local tools and least-privilege publishing
This was referenced Jul 11, 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
2 participants
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: Rename
Tolerance::newtoTolerance::try_newandMatrix::get_checkedtoMatrix::try_get; removeMatrix::set_checkedin favor ofMatrix::set.Vector::dotandVector::norm2_sqnow borrow their operands.det_sign_exactis now infallible and returnsDeterminantSigninstead ofResult<i8, LaError>.LaErrorvariants now use typed reason, location, and origin fields and require..in downstream matches. LDLT now requires exact symmetry, determinant error bounds may be unavailable under gradual underflow, andERR_COEFF_2,ERR_COEFF_3, andERR_COEFF_4are no longer exported by the prelude.Summary by CodeRabbit
New Features
f64conversion APIs (including strict vs rounded conversion), plus a determinant-with-certified-error-bound type.Breaking Changes
Tolerance::newtoTolerance::try_new.try_get/validated mutation;Vector::dotandVector::norm2_sqnow take borrowed inputs.det_sign_exactnow returns aDeterminantSignvalue.Documentation