Repository navigation
Conversation
The `perf-warn` predicate of the column-pivoted factorisation read `col_stride == 1 || row_stride != 1`, so every contiguous row-major input warned "Found matrix with generic strides" -- a layout the message itself names as preferred. Only a contiguous column-major input was silent, because both clauses happen to be false there. Use the predicate the message describes, `has_generic_strides(row_stride, col_stride) = |row_stride| != 1 && |col_stride| != 1`, and unit-test the layouts next to it: contiguous row-major, contiguous column-major and a transposed view are silent, a matrix with neither stride equal to one warns. That is the same predicate as the full-pivoting fix in #342; the helper stays local to this module until both land, then it can move to `lu/mod.rs`. Only the message is affected: no arithmetic path changes, so there is no measurement to make. Upstream `faer` 0.24.4 and its `main` at `0ecd846` carry the same predicate (`faer/src/linalg/lu/partial_pivoting/factor.rs:245-253`). Recorded as `upstream-reports/faer-042-lu-perf-warn-row-major`; nothing was filed upstream, per AGENTS.md's deferred-feedback rule.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: repository code outside the supplied diff, Cargo execution, or live CI; runtime and performance claims remain unverified.Reviewed head: |
`has_generic_strides` reads `|row_stride| != 1 && |col_stride| != 1`, but replacing its body with `row_stride != 1 && col_stride != 1` kept all six cases added with it green: none is a stride of magnitude one on the axis the other one walks, so `unsigned_abs` was untested. Add `(64, -1)` and `(-1, 64)`; with the mutant the first fails (`! has_generic_strides(64, -1) = false`), with the real helper both pass. Drop the unmeasured claim that both preferred layouts factor at the same speed from the helper's doc comment.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: compilation, runtime logging, live CI, or source outside the supplied diff; the test-wiring finding is a source-level mutation analysis, not an executed mutation.Reviewed head: |
…345) The predicate test cannot see the wiring: restoring the old "not column-major" condition at the lu_in_place call site leaves it green. A unit test inside the crate cannot own the warn-once flag either, so add faer/tests/lu_perf_warn.rs, its own binary, with a capturing logger: contiguous row-major and column-major stay silent, a matrix with neither stride equal to one warns once, and a second such factor adds nothing. log is a dev-dependency so the test can install the logger.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: local compilation, test execution, mutation runs, or the final completion of pending CI. Review evidence is limited to the supplied diff and retrieved head files/check statuses.Reviewed head: |
…esult The integration test only compiles under perf-warn, which the test job does not enable, so the call-site regression would never run in CI. Add that invocation. The upstream report checks warning counts only, so its README no longer says the factorisation is correct.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changesnone Unverified claims
Checked and finenone Not checked: compilation, runtime mutation tests, or source outside the supplied diff; remaining risk is unverified execution evidence for the submitted head.Reviewed head: |
Closes #345.
The defect
The
perf-warnblock oflu::partial_pivotingreadwhich is "not contiguous column-major": the first clause is true for a contiguous row-major input
(
col_stride == 1) and the second for anything that is not column-major. So the message"LU with partial pivoting prefers column-major or row-major matrix. Found matrix with generic
strides." fired on every contiguous row-major input, one of the two layouts it recommends. Only
column-major input was silent, because both clauses happen to be false there.
The change
has_generic_strides(row_stride, col_stride) = |row_stride| != 1 && |col_stride| != 1, the predicatethe message describes, used by the warning and unit-tested next to it: contiguous row-major,
contiguous column-major and a transposed view are silent, and a matrix with neither stride equal to one
warns. The two negative unit strides,
(64, -1)and(-1, 64), carry theunsigned_abs: with thehelper body replaced by
row_stride != 1 && col_stride != 1every other case stays green and these twofail (outputs below).
This is the same predicate as the full-pivoting fix in #342. The helper stays local to
partial_pivoting::factoruntil both land; then it belongs inlu/mod.rswith one copy, and #342'scopy can be dropped in the same rebase.
No arithmetic path changes: the gated block is the
log::warn!and nothing else.Verification
Red: the new test against the old predicate, moved into the helper verbatim
The predicate was first transcribed into the helper with its old body
(
col_stride.unsigned_abs() == 1 || row_stride.unsigned_abs() != 1), the test written, and the testrun. It fails on the row-major case, which is the reported defect:
Green: the same test file, helper body fixed
Red: the
unsigned_absmutantHelper body replaced by
row_stride != 1 && col_stride != 1, the mutant the review showed the old sixcases cannot catch:
Green: the same test with the helper restored
The mutant was reverted before this run; the tree is otherwise identical.
cargo test --lib lu::partial_pivoting, full outputThe same filter with the feature that compiles the changed block:
The crate's own suite, and a build of the configuration CI builds first:
cargo checkmatters here because the helper is#[cfg(any(feature = "perf-warn", test))]: a plainnon-test build must not see an unused or dead function, and it does not.
Formatting
Both checks are red on this host at the parent revision too; the tree is not format-clean under the
installed rustfmt, because
rustfmt.tomluses unstable options that stable ignores. The measurementbelow is therefore a before/after count, not a pass (
rustfmt 1.9.0-stable):None of those hunks is on a line this change adds or rewrites (it touches 627-630 and 767-768). CI runs
this step with
continue-on-error: true.Clippy
-D warningsis also red at the parent revision, andcode-quality.ymlstates why the crate is not aclippy gate: "t4a-faer: 2340 warnings on 1.96.0, measured 2026-11-07. Not a gate". What matters here
is that no finding is on an added line:
Upstream
faer0.24.4 and upstreammainat0ecd846(2026-10-22) carry the same predicate, unchanged:So this is an inherited defect and not a fork divergence, and a report is added rather than staying in
the commit message:
upstream-reports/faer-042-lu-perf-warn-row-majorreproduces it against thepublished 0.24.4 crate with a counting
log::Log, exits 1 on a contiguous row-major64 x 64input and0 on the
-- column-majorcontrol:No upstream issue or PR was prepared:
AGENTS.mddefers upstream feedback and leaves that decision tothe maintainer. The full-pivoting sibling (
src/linalg/lu/full_pivoting/factor.rs:477-485upstream) isthe same predicate in another file and is noted inside
faer-042rather than duplicated as its ownentry; the fork fixes that copy in #342.
Not measured
The acceptance grid is not run: the change reaches no arithmetic path, so there is no cell whose
timing it could move. The only code that differs is the condition guarding a
log::warn!.CI on head
99580691Green:
body,build,code-quality,testing-stable-ubuntu-latest,msrv-1.89.0-ubuntu-latest,msrv-1.89.0-windows-latest,nostd-nightly-2025-08-07-ubuntu-latest,nostd-nightly-2025-08-07-windows-latest.Pushed as
99580691on345-lu-partial-perf-warn-row-major; this is PR #347. The body covers thatcommit plus the follow-up commit that adds the negative-unit-stride cases and trims the unmeasured
claim from the helper's doc comment.