Repository navigation
docs(#327): link the fixed-thread-count reproducibility contract, and stop asserting cross-count bitwise equality - #334
Conversation
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Automated ( |
… link in the crate docs #327 withdrew the cross-thread-count bitwise requirement: the promise is bitwise-identical results on the same machine and build at a fixed thread count, and a comparison across counts uses the family's tolerance. `docs/reproducibility.md` (landed with #328) states that; this change makes the tree follow it and links it from the API docs. Three tests asserted exact equality between a `Par::Seq` result and a team result: - `tests/pfaffian.rs::team_agreement_and_actual_row_dispatch` compares the Pfaffian with a bound proportional to the result, `eps x 200 x n x |want|`. The fixtures return ~3e-21 (f64, f32) and ~0.12 (c32, c64); the previous absolute floor made the bound ~1e-11 there, a 1e9 relative error that accepted a negated reference. A negated reference now fails (`width=1: -3.070397953735666e-21 against 3.070397953735666e-21, bound 3.51790895109528e-32`). - `tests/generalized_schur_reorder.rs::team_case` (run by `sequential_and_team_agree`) compares the four factors with the file's `close` tolerance. - `linalg::matmul::tests::test_matmul_with_conj_uninit_team` builds its reference with the same team and compares with `==` (which treats `+0.0` and `-0.0` as equal, so it is representation equality at one width, not a bitwise claim), and compares the serial product against a bound scaled by the product's largest entry. Two exact comparisons stay, each with a comment saying why they are not cross-count claims: a small QR on a team that provably opened no section, and a team of one participant. The `# parallelism` crate docs now state the promise and link `docs/reproducibility.md`. No implementation or public API change; every changed test verifies what it did before.
06c73c1 to
9967d30
Compare
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: Rust tests and mutation commands were not executed; numerical calculations above were checked separately. Automated ( |
…record the pfaffian diffs Three findings from the review of #334, all in test code. `test_matmul_with_conj_uninit_team` folded the scale from 1.0, so the bound was `1e-12 * max(1, largest_entry)` while the comment next to it claimed a relative bound. It now folds from 0.0, asserts the reference is not all zero (a zero reference leaves a zero bound, which demands exact equality), and the same test shrinks the product by 1e-20 where an all-zero result must exceed the bound, which is exactly what the floor made impossible. `max_diff` used `f64::max`, which returns its other operand when one is NaN, so a NaN entry read as agreement. A non-finite difference now passes only where both entries are the same bits, which is what the `+inf` at `q[(3, 31)]` of the 32x32 fixture is on both routes. `team_agreement_and_actual_row_dispatch` prints the difference and the reference per (scalar, width), so "the difference is exactly zero at every width" is a recorded measurement rather than a claim in the description.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
…ds cost Three findings from the review of #334. `max_finite` folds one difference into a running maximum and refuses a non-finite one. `f64::max` returns its other operand when one side is NaN, so an all-NaN output satisfied every tolerance these tests compare against: with `got` filled with NaN, `gemm_tiled_matches_the_gemm_crate_for_every_dtype` passed at the previous head and fails with `non-finite difference NaN` now, and so does the triangular comparison of `triangular_products_on_the_team_match_sequential_and_stay_on_it`. The complex triangular product and the `spicy_matmul` comparisons are `x86_64`-only, so their call sites are converted but cannot be mutated on this host. `max_diff`'s exception for a non-finite difference is now two equal infinities rather than equal bits, which admitted two NaNs of the same bits: their difference is NaN, `f64::max` dropped it, and the measured `max_diff` of a 1x1 NaN against itself was 0.0. Two tests pin both halves: a NaN pair must panic, two `+inf` must still read as zero. The bound of `team_agreement_and_actual_row_dispatch` has no floor, and the complex fixtures are not indifferent to one: their `|want|` is 0.161087, so a floor of 1 was 6.21 times the bound. A 3e-11 relative error on those fixtures passes with the floor and fails against the bound as it stands. `tests/lq_no_alloc.rs` records where its one failure comes from: on a host whose `gemm` selects the aarch64 SME kernel, `PackBuffer::take` builds a `MemBuffer` on every call. That is a dependency path, not this machine's allocator, and it reproduces identically at `620b96a`.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fineRe-ran the full documented release command at Reviewed head: |
The comment gave two different quantities as if they were one: `3.7e9` (f64) and `2.0e18` (f32) are what the floored bound admits as a *relative error*, while `6.21` on the complex fixtures is the ratio of the two bounds. A floor of 1 makes the bound `1 / |want|` times looser, which is `3.2569e20` for `|want| = 3.07e-21` and `6.2078` for `|want| = 0.161087`; the relative-error numbers stay in the PR body's table, where they have their own column.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
The NaN tests added with `max_diff` cover that helper, not `max_finite`, so removing the guard from the manual comparisons left no committed test red: `max_finite(0.0, f64::NAN)` then returns `0.0`, which is the false pass the guard exists to prevent. This adds the direct case, `should_panic` on "non-finite difference"; deleting the `assert!` from `max_finite` makes it fail with "test did not panic as expected".
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
`t4a-gemm-common 0.20.4` keeps the pack buffer of the last call in a thread-local slot and its `Drop` keeps a buffer only up to `CACHE_INFO[1].cache_bytes` (`src/gemm.rs:84-97`, `:114-129`), so "a fresh `MemBuffer` on every call" was wrong about the mechanism. The counted window's allocation sizes say which of those two conditions fires: 3993600 and 4390912 B for f64 (and 7895040, 8781824 B for f32) are above the 2796202 B the crate reports as the cap on this host, so each pass leaves the slot empty, and each smaller request that follows is larger than the buffer then in the slot. That is the 13 allocations; the counts are unchanged by this commit. The `x86_64` half of the sentence stays: that host does not dispatch to the SME kernel.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fineSeven focused tests passed using existing release binaries, not rebuilt libraries; the Pfaffian run reproduced all sixteen reported zero differences. Not checked: a rebuilt full suite or the historical allocation traces. Reviewed head: |
The families test scaled its tolerance by the largest |a| of the reference, and the 32x32 QR fixture holds +inf, so 1e-10 * scale was infinite and any finite error passed. Only finite entries now enter the scale; max_diff still requires an infinity to match on both sides. A regression test compares the fixture's q against a copy whose (0, 0) is off by 1e6 and expects a panic. lib.rs: the width can change both the path a call takes and how work is partitioned (tridiag splits by the participant count), so the old wording "by the problem rather than by the count" contradicted tridiag.rs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The review reproduced the hole on the families test's own column pivoting QR, whose h holds an infinity sentinel. Share that fixture as col_piv_qr and add a regression that rejects h against a copy with (0, 0) off by 1e6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fineRecalculated the Pfaffian table’s bounds and ratios; independently calculated the matmul fixture’s largest absolute entry as Not checked: Rust tests and mutation tests were not rerun during this review; current-head numerical CI was incomplete when inspected. Reviewed head: |
The comment said an all-zero reference leaves a zero bound demanding exact equality, but `assert!(scale > 0.0)` rejects such a reference before any comparison. Say that instead (GPT review of 403f180). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: the full head suite or the complete mutation suite; the comparator probe above is narrower evidence. Reviewed head: |
With an infinity on one side, `close` computed error = inf and bound = inf, and `inf <= inf` accepted the pair, so team_case's comparisons (which replaced assert_eq!) could pass a sequential-only overflow. `close` now requires both to be finite; close_rejects_an_infinite_reference pins it (GPT review of 7f6afed). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changesnone Unverified claims
Checked and fine
Not checked: the full suite, mutation replay, or local x86-only execution; completed current-head hosted numerical coverage remains unverified. Reviewed head: |
Summary
docs/reproducibility.mdalready states the contract that #327 fixed (it landed with #328): bitwise-identicalresults on the same machine and build at a fixed thread count, and nothing across thread counts. This change
links it from the API docs and makes three tests assert what the project actually promises instead of exact
equality between a
Par::Seqresult and a team result. It closes #327.The promise itself is the one in
docs/reproducibility.md(owner decision on #327); this PR links it and doesnot re-establish it. What the tests here check is narrower, and the sections below say which part each one covers.
No test in this repository compares a second process, and nothing added here changes that.
Changed (docs and tests only; no implementation or public API change)
faer/src/lib.rs:183-190# parallelismcrate-doc section states the promise and linksdocs/reproducibility.md(#327); the reason it gives is that a width change can change both the path a call takes and how the work is partitioned (the tridiagonalization splits by the participant count,faer/src/linalg/evd/tridiag.rs:386-409)faer/tests/pfaffian.rs:397-409faer/tests/generalized_schur_reorder.rs:555-561team_casecompares the four factors with the file'sclosetolerance instead ofassert_eq!faer/tests/common/mod.rs:22-31closerefuses a non-finite residual or bound before comparing them: with an infinity on one side both areinf, andinf <= infaccepted the pair. Shared by nine test binariesfaer/tests/generalized_schur_reorder.rs:565-573close_rejects_an_infinite_reference: an 8x8 identity against a copy with(0, 0) = +inf,should_panicon "non-finite"faer/src/linalg/matmul/mod.rs:2477-2510test_matmul_with_conj_uninit_teambuilds its reference with the same team and compares with==; the serial product is compared against a bound scaled by the product's largest entry, and a shrunken copy of the same product checks that the bound rejects an all-zero result. An all-zero reference is outside the test:assert!(scale > 0.0)at:2488rejects it before any comparison, as the comment at:2479-2483now saysfaer/src/utils/team.rs:870-886max_diffaccepts a non-finite difference only between two equal infinities; two NaNs of the same bits fail it (tests at:888-900)faer/src/utils/team.rs:862-868max_finitefolds each difference into a running maximum and refuses a non-finite one, used by every manual comparison in the team testsfaer/src/utils/team.rs:973-977max_finite_rejects_a_nanpins that guard directly,should_panicon "non-finite difference"faer/src/utils/team.rs:903-913assert_within_tolerance; its scale is the largest finite magnitude of the reference (:908-911), so an infinity sentinel no longer makes the bound infinite.max_diffstill requires such an entry to be the same infinity on both sidesfaer/src/utils/team.rs:918-944small_qr, the 32x32 QR whoseqholds+infat (3, 31);col_piv_qr, the families test's 200x200 column pivoting QR) are functions, shared withsmall_qr_on_a_team_is_the_sequential_oneand the families table (:1170)faer/src/utils/team.rs:946-968tolerance_rejects_a_corrupted_col_pivoting_factorandtolerance_ignores_an_infinite_reference_entry: each fixture against a copy whose (0, 0) is off by1e6,should_panicon the tolerance messagefaer/src/utils/team.rs:999-1005,:1948-1950faer/tests/lq_no_alloc.rs:18-30Two notes on wording, because an earlier version of this body overstated them:
assert!(result == expected)in the matmul test is representation equality at one width, not bit equality:==treats+0.0and-0.0as equal.max_diff(...) == 0.0comparisons that stay (a small QR whose team opened no section; a team of oneparticipant) do not distinguish
+0.0from-0.0either. Both sides run the same one-participant partitionthere, which is why an exactly-zero difference is the right assertion.
Why the pfaffian bound changed
At
620b96athis test assertedassert_eq!(got, want); what it asserts now is a bound proportional to theresult. The floor of
1is the alternative that was rejected, and its cost is measured — the four fixtures thistest runs, at
n = 258, bound values printed by the same build:| precision |
|want|| bound with a floor of1| bound as committed | floor is looser by | relative error the floor admits || --- | --- | --- | --- | --- | --- |
| f64 |
3.0703979537356053e-21|1.1457501614131615e-11|3.5179089510952106e-32|3.2569e20|3.7316e9|| f32 |
3.0704116e-21|0.0061511993|1.8886714e-23|3.2569e20|2.0034e18|| c64 |
0.1610874696191822|1.1457501614131615e-11|1.8456599431781575e-12|6.2078|7.1126e-11|| c32 |
0.16108781|0.0061511993|0.0009908832|6.2078|0.0382|So for
f64the floor admits a relative error of ~3.7e9, and forf32~2.0e18— atf32it is not a loosebound but effectively no bound at all. The complex fixtures are not indifferent to it either: their
|want|is0.1611, below one, so the floor was6.21times the bound there. (Two corrections from the review ofce7ec19: the earlier version of this section said the floor "never mattered" for the complex fixtures, which isnumerically wrong, and it described that floor as the previous bound, which no commit on this branch carried.)
Measured difference per (scalar, width)
cargo test -p t4a-faer --features threadteam --release --test pfaffian team_agreement_and_actual_row_dispatch -- --nocaptureprints one line per case; the values below are copied from that output, not rounded. The test asserts the bound,
not equality with zero — the zero is recorded, so that the claim can be re-measured rather than believed:
Mutation checks
Each fix below was measured by breaking the line it fixes, at
0136bcaor at the head it was written on;those lines have not changed since.
1. A NaN through
max_diff(the exception put back toto_bits()equality):With the
to_bits()exception a1 x 1NaN against itself reads asmax_diff = 0.0; with theequal-infinities exception it panics with
0,0: NaN and NaN differ by NaN, and two+infstill read as0.0,which is what the 32x32 QR fixture needs at
q[(3, 31)].2. An all-NaN tiled product (
gotfilled with NaN just before the comparison):The same injection against the previous
fold(0.0, f64::max)passes, which is the hole this closes:f64::maxreturns its other operand when one side is NaN, so an all-NaN output satisfied the
1e-3tolerance. The same twodirections measured on the triangular comparison (
got_l/got_ufilled with NaN intriangular_products_on_the_team_match_sequential_and_stay_on_it):max_finite→FAILED, the previousd.max(...)→ok. The complex triangular product (:1990-1996) and thespicy_matmulcomparisons(
:2019-2025,:2234,:2296) sit inx86_64-only tests, so their call sites are converted but could not bemutated on this aarch64 host.
3. A relative error the floor would have admitted (the complex fixtures given a deliberate
3e-11relativeerror,
got + want * 3e-11):So the floor was not inert on the complex fixtures: it admitted an error
6.21times larger than the bound thatships.
4. The matmul bound floor (from the earlier round, still valid at this head, unchanged since
ce7ec19): withfold(0.0f64, ...)restored tofold(1.0f64, ...),test_matmul_with_conj_uninit_teamfails with0.00000000000000000101 against 0.000000000001: the bound admits an all-zero product.5. The
max_finiteguard (theassert!deleted frommax_finite):Without the guard
max_finite(0.0, f64::NAN)returns0.0, which is the false pass the guard prevents;with it the test passes (
3 passed; 0 failedforutils::team::tests::max_).All five mutations were reverted;
git statusis clean at0136bca.What the tests do and do not establish
eps x 200 x nscaled by the result; where an exact comparison is kept, both sides run the same partition, so the difference is
zero by construction.
docs/reproducibility.md:15-19says no test in this tree compares a second process; this PR does not add one anddoes not claim the property on the strength of these tests.
Verification
Head
6ae77d2: the needs-change of the review of7f6afedaarch64 macOS host,
rust-toolchain.toml= 1.96.0. Output copied from the runs.faer/tests/common/mod.rscloseacceptederror = infagainstbound = inf. The new test fails without thefix (
git stash push -- faer/tests/common/mod.rs, test file kept):With the fix (
git stash pop):closeis shared, so every binary that declaresmod commonwas run with the fix, to check that none of themrelied on comparing infinities:
Hosted checks on
6ae77d2: pending; this section is updated with the run ids onceRun Testsandcicomplete.Head
7f6afed: the needs-change of the review of403f180A comment-only change in
faer/src/linalg/matmul/mod.rs:2482-2483: the comment said an all-zero reference leavesa zero bound that demands exact equality, while
assert!(scale > 0.0)(:2488) rejects that reference first. Itnow says the reference is outside the test and rejected by that assert. No assertion changed, so there is no
red run to show. aarch64 macOS host,
rust-toolchain.toml= 1.96.0:Superseded by
6ae77d2before its hosted runs finished; the hosted checks are recorded for6ae77d2above.Head
403f180: the two needs-changes of the review of0136bcaaarch64 macOS host,
rust-toolchain.toml= 1.96.0. Output copied from the runs.The new tests fail without the fix. With the scale line put back to its
0136bcaform(
.fold(1.0f64, |m, (i, j)| m.max(a[(i, j)].abs()))) and everything else at403f180:Each test asserts its sentinel is present before the comparison, so "did not panic" means the
1e6errorwas accepted, not that the fixture changed. With the fix:
(The seven lines are the comparison helpers and the tests that use them, picked from the 34.)
cargo clippy -p t4a-faer --all-targetsexits 101 with the crate's pre-existing errors(
could not compile t4a-faer (lib) due to 40 previous errors,(lib test) due to 41), none infaer/src/lib.rs:184-188or the changed lines of
faer/src/utils/team.rs; the blocking clippy job lintst4a-faer-traitsonly(
code-quality.yml:34-35).cargo fmt --checkreports 702 hunks at0136bcaand 703 here: the moved QRcall lines keep the file's long-line style, and CI's formatting step is advisory (
code-quality.yml:26-32).The full suite was not rerun for this change, which touches tests and a doc comment only.
Head
0136bcaHead
0136bca, on an aarch64 macOS host,rust-toolchain.toml= 1.96.0. Output copied from the runs.Per-target counts from the same run:
The one failing test
It is pre-existing and not from this change, and where it comes from is measured rather than assumed:
620b96a, the base of this PR, with the same command and build settings:left: [13, 12, 17, 17], 7 passed and this one failed.dyn_stack::mem::MemBuffer::newinside
gemm_common::gemm::PackBuffer::take, reached fromgemm_f64::gemm::f64::sme::gemm_basic←faer::linalg::matmul::matmul_with_conj←householder::apply_block_householder_on_the_left_in_place_generic←lq::no_pivoting::unitary::unitary.That is the
gemmcrate's aarch64 SME kernel. Onx86_64this dispatch does not exist, which is why the sametest passes on the
ubuntu-latestjob of run42550520977for9967d30.pack buffer is per thread but is only reused when the buffer in the slot can hold the request
(
t4a-gemm-common 0.20.4,Cargo.lock:2709-2712,src/gemm.rs:84-97), andDropkeeps a buffer only up toCACHE_INFO[1].cache_bytes(src/gemm.rs:114-129). A two-line program over the pinned version prints that capon this host as
L2 bytes=2796202. The60 x 300window allocates 3993600 and 4390912 B for f64 (and 7895040,8781824 B for f32), both above the cap, so each pass ends with the slot empty, and each smaller request that
follows (57344, 114688, 65536, 131072, 147456 B) is larger than the buffer then in the slot. That is the 13
allocations of f64, in the order measured.
faer/tests/lq_no_alloc.rs:18-30records this third inherited path next to the two it already documented. Anearlier version of this section said "the allocation counter on this machine's allocator", and the one after it
said the kernel "takes its pack buffer per call" without saying which of the two conditions above fires; both
are withdrawn.
The two probes, both uncommitted and reverted after the run:
Reported rather than hidden; no
--skipwas added to make the run look green.Hosted checks on the reviewed head
0136bca, all completed successfully (gh pr checks 334, each run'sheadShachecked as0136bca):ci42570326469buildjob, 15m58s)Run Tests42570326466testing-stable-ubuntu-latest28m04s, which runs the numerical tests withnextestonx86_64, so it executes the x86-only team comparisons this host skips;msrv-1.89.0andnostd-nightlyalso greenAssess Code Quality42570326461PR verification section42570326763Not measured: no performance claim is made, and the measured table in
docs/reproducibility.mdis quoted from#327 rather than re-run here.
cargo fmtis not run: the tree is formatted with nightly rustfmt options, CI'sformatting step is advisory (
code-quality.yml:28-32), and a stablecargo fmtrewrites unrelated files.cargo clippy -p t4a-faer --all-targets --features threadteamreports no diagnostic on any line this PR adds(the crate's pre-existing diagnostics,
code-quality.yml:38-40, are untouched).Every path and line pin that
docs/reproducibility.mdcites (faer/src/utils/cost.rs:613-619,:711-713,faer/src/linalg/matmul/internal/mod.rs:252-278,:289-293,faer/src/utils/team.rs:515,faer/src/linalg/evd/tridiag.rs:386-409,:457-459,faer/tests/lu_rank_truncation.rs:183-218,PATCHSET.md:135)resolves at this revision, and
docs/new-routine-checklist.mdexists and is linked.Review
review: gpt-6.1-sol, 2026-11-09T04:08Z on
ce7ec195: 0 critical, 3 needs-changes, 1 unverified — all fouraddressed by the mutation checks and the base-vs-head measurement above.
review: gpt-6.1-sol, 2026-11-09T06:07Z on
0136bca7: 0 critical, 2 needs-changes, 2 unverified. Bothneeds-changes are addressed at
403f180: the infinite tolerance scale (faer/src/utils/team.rs:908-911, regressionsat
:946-968, red and green runs above) and the "by the problem rather than by the count" sentence(
faer/src/lib.rs:184-188). The two unverified items (hosted run of the reviewed head; the allocation trace) areunchanged by this push.
review: gpt-6.1-sol, 2026-11-09T07:25Z on
403f180: 0 critical, 1 needs-changes, 1 unverified. The needs-change(the all-zero-reference comment against
assert!(scale > 0.0)infaer/src/linalg/matmul/mod.rs) is addressedat
7f6afed. The unverified item (no completed numerical run on the reviewed head) is recorded underVerification once the hosted runs finish.
review: gpt-6.1-sol, 2026-11-09T07:40Z on
7f6afed: 0 critical, 1 needs-changes, 2 unverified. The needs-change(
closeacceptinginfagainstinf, reached by the Schur team comparisons) is addressed at6ae77d2(
faer/tests/common/mod.rs:22-31, regression atfaer/tests/generalized_schur_reorder.rs:565-573). Unverified:the hosted run of the head is recorded under Verification once it finishes; the allocation trace is unchanged by
this push.