Skip to content
This repository was archived by the owner on Aug 20, 2026. It is now read-only.

bounds: correct transpose() docstring — Replicate is NOT exact for Lame/bending - #43

Merged
balbasty merged 1 commit into
mainfrom
claude/jitfields-fastfields-migration-v5r416
Aug 2, 2026
Merged

bounds: correct transpose() docstring — Replicate is NOT exact for Lame/bending#43
balbasty merged 1 commit into
mainfrom
claude/jitfields-fastfields-migration-v5r416

Conversation

@balbasty

@balbastybalbasty commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Workstream: claude-jitfields-to-fastfields

Context

Independent correctness audit of the recent self-adjointness fixes to the JRLS regularisers (matvec_lame_jrls#31, bending RLS/JRLS #38). Both fixes are verified genuinely correct — see verification below — but auditing them surfaced a separate, pre-existing documentation bug in bounds.h's transpose() comment (added in f39ee7b), which this PR corrects. No functional/code change — comment only.

The bug

f39ee7b documented transpose() as yielding an exactly self-adjoint Lamé/bending operator "for DFT / Zero / Replicate / NoCheck" (alongside DCT2↔DST2). That claim is wrong for Replicate.

Verification

Built the explicit operator matrix column-by-column (L eⱼ for every basis vector eⱼ, i.e. a strictly stronger check than the existing <Lx,y> == <x,Ly> bilinear-form oracle) for matvec_lame/matvec_all (flow) and matvec_bending (field), 2D and 3D, with and without JRLS weighting, across all 8 bound::type values:

  • Zero, DCT2, DST2, DFT, NoCheck: max|L - L^T| / max|L| is exactly 0 (or at float64 rounding noise, ~1e-16) in every configuration tested, including the 3D lame_jrls path that the CPU test suite doesn't cover at all. The two audited self-adjointness fixes are genuinely correct here.
  • DCT1, DST1: asymmetric, as already correctly documented (excluded from the CPU suite's symmetry assertions with a clear rationale).
  • Replicate: also asymmetric (max|L-L^T|/max|L| ≈ 0.03–0.5 depending on configuration) — but undocumented, and silently absent from the RLS/JRLS symmetry tests (which only cover Zero/DCT2/DST2/DFT). The same asymmetry reproduces bit-for-bit with no weight map at all (the plain, non-JRLS operator), proving this is a structural property of transpose(Replicate) == Replicate predating both audited commits, not something the JRLS fixes introduced or missed. Only the cross-channel Lamé and 2nd-order bending stencils are affected — absolute/membrane stay exactly self-adjoint under Replicate.

Fix

Update the comment to move Replicate into the same documented-limitation bucket as DCT1/DST1, with the numeric evidence and rationale (clamp has no parity to flip, but clamping is not its own adjoint for a difference operator — same structural gap). Also confirmed (separately, via git ls-tree/git blame) that:

  • The kernels submodule pins in fastfields-cpu-impl/fastfields-cuda-impl correctly point at commits including both fixes, gitlinks are still mode 160000 in both.
  • fastfields-cuda-lib's post-rewrite (PR regularisers/flow: fix channel strides typed as reduce_t in kernel_all #28) reg_field.cpp/reg_flow.cpp still contain correct field_precond/field_forward/flow_precond/flow_forward logic mirroring the CPU dispatch, with the try { sym_solve(...) } catch(...) { freeDevice(...); throw; } pattern intact on every exit path, no duplicate definitions across the reg_field.cpp/reg_field_rls.cpp split, and no dead files. CI timeout-minutes: 120 / -O1 / -j2 / BOUNDFLAGS are internally consistent with the current Makefile.

Not done here (flagging for human judgement, not fixing)

Whether Replicate should be excluded from the regularisers (like a hard-rejected bound), given a test-suite note like the DCT1/DST1 one, or left as an accepted "documented but not solver-safe" option is a design call, not a bug fix — left to human review rather than acting on it unilaterally.


Generated by Claude Code

…me/bending
The comment added in f39ee7b claimed transpose() yields an exactly
self-adjoint operator "for DFT / Zero / Replicate / NoCheck", alongside the
DCT2<->DST2 pair. That is wrong for Replicate.
Verified by building the explicit matvec_lame/matvec_all (flow) and
matvec_bending (field) matrices column-by-column under bound::type::Replicate,
2D and 3D, with and without JRLS weighting: max|L - L^T| / max|L| is ~0.03-0.5
(clearly asymmetric, not rounding noise). The same asymmetry reproduces with
no weight map at all, so this is a pre-existing gap in the boundary-transpose
scheme itself, not something the matvec_lame_jrls / bending_rls-jrls
self-adjointness fixes (#31, #38) introduced or could have caught — Replicate
is absent from every RLS/JRLS symmetry test in fastfields-cpu-lib's suite
(which covers Zero/DCT2/DST2/DFT only), so the gap has been silent.
Root cause: transpose(Replicate) == Replicate (clamp has no parity to flip),
but clamping is not actually its own adjoint for a first-difference operator —
the same structural issue already correctly documented for DCT1/DST1. Only the
cross-channel Lame and 2nd-order bending stencils are affected; absolute and
membrane stay exactly self-adjoint under Replicate.
No functional change — this is a comment-only fix so the documented behaviour
matches the (unchanged) code. Flagging for human review per the standing
audit process; not self-merging.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
@balbasty

Copy link
Copy Markdown
CollaboratorAuthor

Agreed -> implement.

Symmetry-breakoung boundaries should not be part of the statically compiled suite (only runtime, in case they are ever useful).

@balbasty
balbastyforce-pushed the claude/jitfields-fastfields-migration-v5r416 branch from b998434 to 09de249CompareJuly 30, 2026 21:54
@balbasty

Copy link
Copy Markdown
CollaboratorAuthor

Let's keep it, documented as "not safe". For now.

balbasty added a commit that referenced this pull request Aug 1, 2026
…#58)
`supports_bending` shipped in #56 with the rejection set from #50's
ORIGINAL Decision 2, {Replicate, DCT1, DST1}. Two independent
from-scratch operator-matrix measurements -- mine on #56, then an
adversarial reviewer's on different grids -- agree it is wrong in both
directions, and #50's Decision 2 has been rewritten accordingly:
* DST1 must NOT be rejected for field's bending. Its +-2 fold lands
back on the centre voxel (a diagonal entry) and its +-1 fold hits the
sign-0 phantom node, so no unmatched off-diagonal entry is created.
Measured 0 relative asymmetry, every D, to the last bit.
* Reach-1 energies are NOT universally self-adjoint, which #43 assumed
and nothing checked. `membrane` measures 0.25-0.46 relative asymmetry
under DCT1, by the same whole-sample-fold mechanism as bending's DCT1
failure. That case had no predicate at all.
The mechanism is about REACH -- more reach folds more taps, so it can
only ever lose conditions -- so key on that and name the energies on top:
supports_reach(b, reach)
supports_absolute/supports_membrane/supports_bending
Corrected set, from measurement:
bound | absolute | membrane | bending
-----------+----------+----------+---------
Zero | ok | ok | ok
Replicate | ok | ok | REJECT (0.042-0.13)
DCT1 | ok | REJECT | REJECT (0.25-0.46 / 0.37-0.50)
DCT2 | ok | ok | ok
DST1 | ok | ok | ok
DST2 | ok | ok | ok
DFT | ok | ok | ok
NoCheck | ok | ok | ok
`absolute` is measured too rather than waved through as "diagonal, so
obviously fine" -- that is the same shape of argument that was wrong
twice above. It is exact under all eight (it reads no neighbour, so there
is no fold to be non-involutive), on a fresh grid, every D, C = 1 and 2.
That table is `static_assert`ed in the header, so it cannot drift away
from the measurement the next time someone edits the prose around it, and
the superseded claims (#43's, #50's original) are recorded in the comment
so they are not re-derived.
Scope is deliberately FIELD ONLY: flow's membrane/bending share this
separable per-component stencil, but Lame's cross-channel block folds
through `transpose(B)` and needs its own measurement before it gets a
predicate (#50 phase 2). Extending these by assumption is exactly the
move that produced both corrections.
No call sites change: nothing in the tree consumed the predicate yet.
`make -C fastfields-cpu-lib test` on clang++ and g++ stays green (the
matching dispatch-entry rejection is fastfields-cpu-lib#54).
Part of #50.
Co-authored-by: Claude <noreply@anthropic.com>
@balbasty
balbasty merged commit 501fce0 into mainAug 2, 2026
2 checks passed
@balbasty
balbasty deleted the claude/jitfields-fastfields-migration-v5r416 branch August 2, 2026 20:17
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@balbasty@claude