test(reg): cover the diag_bending/diag_all corner cross-term bug by balbasty · Pull Request #53 · fastfields/fastfields-cpu-lib

balbasty · 2026-08-01T08:58:05Z

Agent: claude-fastfields-to-teeny

Relates to fastfields/fastfields-kernels#48 and fastfields/fastfields-kernels#49.

This PR does not close either issue and does not fix anything. It is test coverage only for a bug whose fix lives in a different repo (fastfields-kernels PR #49). No production code in this repo is touched — the diff is two test files, additions only. fastfields-kernels has no test suite of its own, so fastfields-cpu-lib is the only place this regression can be guarded.

⚠️Merge ordering — this PR's CI will be red until kernels#49 lands and the pins are bumped. See Merge ordering below. That red is the point of the PR, not a defect in it.

What the bug was

diag_bending (field) and diag_bending / diag_all (flow) expanded the boundary-corrected corner weight as

w * (fx0*fy0 + fx1*fy0 + fx1*fy0 + fx1*fy1)

fx1*fy0 counted twice, fx0*fy1 dropped. The correct expansion of (fx0+fx1)*(fy0+fy1) is fx0*fy0 + fx1*fy0 + fx0*fy1 + fx1*fy1, which is what matvec_bending / matvec_all already computed. Fixed in fastfields/fastfields-kernels#49 (12 terms across 6 functions).

What the tests do

Three functions, no independent reference implementation needed — they exploit a symmetry the buggy expression breaks:

test	file	invariant
`test_diag_boundary_symmetry_2d`	`tests/test_reg_field.cpp`	`field_diag`: on a square domain with the same BC on every axis, `diag(0,j) == diag(j,0)`
`test_diag_boundary_symmetry_2d_bending`	`tests/test_reg_flow.cpp`	`flow_diag` with `shears = div = 0`: channels are independent, so `diag(0,j,c) == diag(j,0,c)`
`test_diag_boundary_symmetry_2d_all`	`tests/test_reg_flow.cpp`	`flow_diag` via the combined `diag_all` path with `shears == div`: the Lamé terms are symmetric under simultaneous axis and channel swap, so `diag(0,j,c) == diag(j,0,1-c)`

All three use a 6×6 domain under DST2 (sign-flipping) with bending active. The buggy corner sum is asymmetric under fx <-> fy relabelling wherever the two axes' one-sided boundary signs differ — it cancels exactly at fully-symmetric corners, which is precisely why the existing interior-only diag tests never caught it.

DST2 is compiled statically under the make test default BOUNDFLAGS (-DFF_STATIC_BOUND_DCT2=1 -DFF_STATIC_BOUND_DST2=1) and is one of the two static conditions in the cuda-default CI leg, so these tests exercise a real shipped instantiation in every matrix leg.

Revert/restore cycle — the whole point of this PR

Run on this exact branch, with only the kernels submodule swapped between the two runs (impl held at b6eae48, main's pin):

kernels commit	`test_reg_field`	`test_reg_flow`	suite
`15295c2` — pre-fix (`main`'s current pin)	4616 checks, 4 failures	5883 checks, 16 failures	FAILED
`9b6517f` — post-fix (head of kernels#49)	4616 checks, 0 failures	5883 checks, 0 failures	all 12 binaries PASS

All 20 pre-fix failures are the new checks, and none of them are marginal:

MISMATCH [diag_bending.boundary_symmetry_2d]: got 12.7 expected 20.7
MISMATCH [diag_bending.boundary_symmetry_2d]: got 10.7 expected 18.7
MISMATCH [flow2d_diag_bending.boundary_symmetry]: got 12.7 expected 20.7
MISMATCH [flow2d_diag_bending.boundary_symmetry]: got 10.7 expected 18.7
MISMATCH [flow2d_diag_all.boundary_symmetry]: got 14.1 expected 22.1
MISMATCH [flow2d_diag_all.boundary_symmetry]: got 16.9 expected 24.9
...

A uniform ~8.0 discrepancy at the affected boundary voxels — far outside the 1e-5 tolerance, so this is a decisive detector rather than a tolerance-sensitive one. No other suite changes: the remaining ten binaries pass identically in both runs.

Merge ordering

CI checks out submodules recursively at their pinned commits, and main's chain is cpu-lib -> impl b6eae48 -> kernels 15295c2 — the pre-fix kernels. So on main today these tests fail by construction. Merge sequence:

fix(regularisers): correct diag_bending/diag_all corner cross-term sign error fastfields-kernels#49 (the actual fix) merges to kernels main
fastfields-cpu-impl bumps its kernels pin
fastfields-cpu-lib bumps its impl pin
then this PR merges (or is rebased onto the bumped main) and goes green

Merging this before step 3 turns the CI gate red for everyone. I deliberately did not bump any submodule pin here — pin bumps are their own change, and bundling one into a test-only PR would hide a dependency update inside a test diff.

Generated by Claude Code

Adds three boundary-symmetry regression tests for the corner cross-term sign error fixed upstream in fastfields-kernels#49 (issue fastfields-kernels#48): diag_bending's (and flow diag_all's) boundary correction expanded the corner weight as (fx0*fy0 + fx1*fy0 + fx1*fy0 + fx1*fy1) -- fx1*fy0 twice, fx0*fy1 dropped. These are TEST COVERAGE ONLY; the fix itself lives in the kernels repo. Nothing in this repo changes behaviour. The tests need no independent reference implementation. On a square domain with the same boundary condition on every axis (DST2, bending active), the operator and hence its diagonal must be symmetric under axis swap, so diag(0,j,c) == diag(j,0,c); for diag_all with shears == div the Lame terms are symmetric under simultaneous axis and channel swap, so diag(0,j,c) == diag(j,0,1-c). The buggy corner sum is asymmetric under fx<->fy relabelling wherever the two axes' one-sided boundary signs differ, which is exactly what the existing interior-only diag tests could never see. Verified by the full revert/restore cycle against this same tree: kernels @ 15295c2 (pre-fix) -> test_reg_field 4 failures, test_reg_flow 16 failures kernels @ 9b6517f (post-fix) -> both PASS, whole suite 0 failures Relates to fastfields-kernels#48, fastfields-kernels#49 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xcQBMEdA5eQtTsx2wB3AC

Adds the {field,flow}_{diag,kernel}_{add_,sub_} in-place entry points, which were lost in the port from jitfields. There, every regulariser entry point is templated on `char op` (Op<'='> = set, '+' = iadd, '-' = isub, see jitfields/csrc/lib/regularisers/{field,flow}/utils.h) and writes *through* `out`, so the add/sub forms are in-place only at the C level -- there is no separate "return a fresh tensor" C entry point. The `char op` templating survived the port in kernels/ and cpu-impl; only the surface was missing. _field_diag / _field_kernel / _flow_diag / _flow_kernel hardcoded '=' and are now op-templated, with ADD_/SUB_ dispatch macros mirroring the existing ADD_MV_DT / SUB_MV_DT. Also renames the four symbols added by task #53, {field,flow}_matvec_{add,sub} -> ..._{add_,sub_}: they are in-place only, and ff:: already marks accumulate-into-out with a trailing underscore (sym_addmatvec_, sym_submatvec_, and the field_precond / field_precond_ pair). Without it the name would also collide with the *out-of-place* fastfields.{numpy,torch,cupy}.field_matvec_add. Tests: run_2d_diag_kernel_addsub in tests/test_reg_{field,flow}.cpp, in the same oracle style as the existing run_2d_matvec_addsub -- the '=' path computes the reference, then add_/sub_ must reproduce base +/- ref against a *nonzero* pre-existing buffer, which is what catches an op that silently overwrites. Covers absolute/membrane/bending, the flow Lame matrix-stencil shape, three boundary conditions, and float+double. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z

…m jitfields) (#50) * reg: restore in-place diag/kernel accumulate + rename matvec accumulate Adds the {field,flow}_{diag,kernel}_{add_,sub_} in-place entry points, which were lost in the port from jitfields. There, every regulariser entry point is templated on `char op` (Op<'='> = set, '+' = iadd, '-' = isub, see jitfields/csrc/lib/regularisers/{field,flow}/utils.h) and writes *through* `out`, so the add/sub forms are in-place only at the C level -- there is no separate "return a fresh tensor" C entry point. The `char op` templating survived the port in kernels/ and cpu-impl; only the surface was missing. _field_diag / _field_kernel / _flow_diag / _flow_kernel hardcoded '=' and are now op-templated, with ADD_/SUB_ dispatch macros mirroring the existing ADD_MV_DT / SUB_MV_DT. Also renames the four symbols added by task #53, {field,flow}_matvec_{add,sub} -> ..._{add_,sub_}: they are in-place only, and ff:: already marks accumulate-into-out with a trailing underscore (sym_addmatvec_, sym_submatvec_, and the field_precond / field_precond_ pair). Without it the name would also collide with the *out-of-place* fastfields.{numpy,torch,cupy}.field_matvec_add. Tests: run_2d_diag_kernel_addsub in tests/test_reg_{field,flow}.cpp, in the same oracle style as the existing run_2d_matvec_addsub -- the '=' path computes the reference, then add_/sub_ must reproduce base +/- ref against a *nonzero* pre-existing buffer, which is what catches an op that silently overwrites. Covers absolute/membrane/bending, the flow Lame matrix-stencil shape, three boundary conditions, and float+double. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z * reg: rename accumulate entry points to verb-first (addmatvec_/adddiag_/...) Repo-owner decision: the C++ and Python surfaces should be consistent with each other and with this codebase's own existing convention, even where that means diverging from jitfields' own naming (jitfields itself keeps field_matvec_add/field_matvec_add_, noun-first). Renames the 12 public ff:: entry points restored in the previous commit on this branch: {field,flow}_matvec_add_ -> {field,flow}_addmatvec_ {field,flow}_matvec_sub_ -> {field,flow}_submatvec_ {field,flow}_diag_add_ -> {field,flow}_adddiag_ {field,flow}_diag_sub_ -> {field,flow}_subdiag_ {field,flow}_kernel_add_ -> {field,flow}_addkernel_ {field,flow}_kernel_sub_ -> {field,flow}_subkernel_ This extends the verb-first + trailing-underscore convention already used for posdef (sym_addmatvec_ / sym_submatvec_) to reg_field/reg_flow, rather than inventing a new one. Internal dispatch macro names (ADD_MV_DT, ADD_DG_DT, ADD_KN_DT, ...) are untouched -- only the public ff:: symbols move. field_matvec / field_diag / field_kernel (the plain, non-accumulate ops) and field_relax / field_forward / field_precond{,_} / field_matvec_rls / field_diag_rls / field_relax_rls (and flow equivalents) are out of scope and unchanged. Verified with a full rebuild + test run, not just a mechanical find/replace: make test, all 12 binaries, 0 failures (identical check counts to the pre-rename commit -- 7426/11415/186 for reg_field/reg_flow/reg_op). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z --------- Co-authored-by: Claude <noreply@anthropic.com>

…loses#65 (#73) * fix: correct field_rls_is_jrls RLS/JRLS predicate (was backwards) field_rls_is_jrls() had the two cases exactly backwards relative to the original jitfields/nitorch semantics: RLS = per-channel weight (wc == nc), JRLS = one shared weight broadcast across channels (wc == 1). The predicate labelled wc==1 as RLS and wc==nc as JRLS, routing a genuine per-channel weight map to the broadcast-only _jrls kernels, which silently discarded channels 1..nc-1 in favor of channel 0's weight. Bump the impl pin to fastfields-cpu-impl's revert of #48/#53 (which themselves undid the wrong-direction consequences of this mislabel: #53 threaded a per-channel stride into _jrls thinking wc==nc needed it, and #48 forced wsc=0 in _rls thinking wc==1 needed broadcasting -- both backwards). With the predicate corrected and the impl/kernels layers back to their pre-#48/#53 state, wc==nc now correctly reaches the genuine per-channel _rls kernels and wc==1 reaches the broadcast _jrls kernels. Add run_2d_rls_jrls_per_channel: a wc==nc weight map with genuinely different values per channel, checked against an independent oracle (decomposition into C single-channel wc==1 problems, a different, already-heavily-tested dispatch branch) -- not a self-consistency check. None of the existing tests catch this because they either use an all-ones weight (invisible to a broadcast bug) or only check self-adjointness (symmetric regardless of which value is broadcast). Closes#65. * style: run clang-format on the RLS/JRLS predicate fix and new test Mechanical formatting only (alignment, pointer-space style, line wrapping) -- no semantic change. Re-verified with a full make test run: 12/12 suites pass, test_reg_field 13360 checks, 0 failures. --------- Co-authored-by: Claude <noreply@anthropic.com>

balbasty · 2026-08-18T15:07:06Z

Closing this — not because the test was wrong. It wasn't: the oracle is sound, and I reused the idea directly.

Why it's being closed

This PR comes from the claude-fastfields-to-teeny workstream, whose PRs must always target the teeny branch. This one targets main, so it was mis-filed. That's the sole reason.

The oracle was sound — credit where it's due

The key insight here is a good one and needs no reference implementation: on a square domain with the same boundary condition on every axis, the operator's diagonal must be symmetric under an axis swap, so diag(0,j,c) == diag(j,0,c); and for diag_all, where the Lamé terms couple the channels, the axis swap carries a channel swap, so diag(0,j,c) == diag(j,0,1-c) with shears == div. That framing is exactly right and I adopted it.

Where the coverage actually landed

On main via #89 (merged, squashed as 1fb2b37), which is deliberately broader than this PR:

3D as well as 2D. 9 of the 12 corrected terms live in the 3D kernels (w110/w101/w011 in field/3d.h and flow/3d.h); a 2D-only test leaves most of the fix uncovered. The xy and yz swaps exercise two cross-terms directly and the third by composition.
Whole-domain sweep rather than row 0 vs column 0.
Three boundary conditions — Zero, DCT2, DST2 — with DCT2 kept as an explicit control, since it does not flip signs and passes either way.

It demonstrably bites

Same tests, same flags; only the kernels headers differ:

kernels	`test_reg_field`	`test_reg_flow`
pre-fix	19250 checks, 328 failures → FAILED	16347 checks, 1696 failures → FAILED
`1df9fd3`	19250 checks, 0 failures → PASSED	16347 checks, 0 failures → PASSED

Check counts identical in both directions, so only pass/fail differs, and all 2024 failures are in the new checks — no pre-existing test changes value.

No follow-up issue needed

I checked teeny rather than assuming: 8afcab4 is on teeny, and both test_diag_boundary_symmetry_2d (in tests/test_reg_field.cpp) and test_diag_boundary_symmetry_2d_bending / _2d_all (in tests/test_reg_flow.cpp) are present and wired into main() at teeny HEAD. So this regression test exists on bothmain and teeny, and there is no gap to track — I have deliberately not opened an issue that would be a duplicate.

The one thing teeny lacks is the extra 3D / whole-domain / multi-bound coverage that #89 added on main. That is an enhancement of my own, not stranded work from this PR, so I have left it as a note here rather than filing an issue for it — happy to open one if you'd like that coverage carried across.

This PR's branch has not been modified.

Generated by Claude Code

balbasty added the claude-fastfields-to-teeny label Aug 1, 2026 — with Claude

balbasty mentioned this pull request Aug 1, 2026
fix(regularisers): correct diag_bending/diag_all corner cross-term sign error fastfields/fastfields-kernels#49
Closed

balbasty marked this pull request as draft August 1, 2026 08:59

balbasty mentioned this pull request Aug 1, 2026
perf(reg_flow): route bending==0 Lamé calls to matvec_lame/diag_lame #52
Closed

balbasty closed this Aug 18, 2026

Navigation Menu

Uh oh!

test(reg): cover the diag_bending/diag_all corner cross-term bug - #53

test(reg): cover the diag_bending/diag_all corner cross-term bug#53
balbasty wants to merge 1 commit into
mainfrom
claude/reg-diag-boundary-symmetry-tests

balbasty commented Aug 1, 2026

Uh oh!

balbasty commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

balbasty commented Aug 1, 2026

What the bug was

What the tests do

Revert/restore cycle — the whole point of this PR

Merge ordering

Uh oh!

balbasty commented Aug 18, 2026

Why it's being closed

The oracle was sound — credit where it's due

Where the coverage actually landed

It demonstrably bites

No follow-up issue needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants