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

reg: restore the in-place diag/kernel accumulate primitives (lost from jitfields) - #50

Merged
balbasty merged 2 commits into
mainfrom
claude/reg-accumulate-inplace-restore
Aug 1, 2026
Merged

reg: restore the in-place diag/kernel accumulate primitives (lost from jitfields)#50
balbasty merged 2 commits into
mainfrom
claude/reg-accumulate-inplace-restore

Conversation

@balbasty

Copy link
Copy Markdown
Contributor

Changes (fastfields-cpu-lib)

  • _field_diag / _field_kernel / _flow_diag / _flow_kernel are now
    templated on char op (they hardcoded '='); the impl layer already took the
    op, so this only threads it through.
  • New ADD_DG_DT / SUB_DG_DT / ADD_KN_DT / SUB_KN_DT dispatch macros,
    mirroring the existing ADD_MV_DT / SUB_MV_DT.
  • New public entry points: {field,flow}_{diag,kernel}_{add_,sub_} (8).
  • Renamed the four task-test(reg): cover the diag_bending/diag_all corner cross-term bug #53 symbols {field,flow}_matvec_{add,sub} ->
    ..._{add_,sub_} (see Naming below), including the internal
    field_forward / flow_forward call sites.
  • Tests: run_2d_diag_kernel_addsub in tests/test_reg_{field,flow}.cpp,
    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 Lamé
    (matrix-stencil) shape, three boundary conditions, and float+double.

Note tests/test_reg_op.cpp already covered impl-level op dispatch for
matvec and diag; the kernel op path had no coverage until now.

Verification

make test CXX=clang++ — all 11 test binaries pass, 0 failures:

test_reg_field checks: 7426, failures: 0 PASSED
test_reg_flow checks: 11415, failures: 0 PASSED
test_reg_op checks: 186, failures: 0 PASSED
(+ distance, distance_mesh, distance_spline, posdef, pushpull,
resize, restrict, splinc — all PASSED)

Background (shared by this PR series)

The regulariser accumulate ops ({field,flow}_{matvec,kernel,diag} with
op = set / add / sub) were in-place-only at the C level in jitfields, and
that primitive was lost partway up the fastfields port. An earlier PR pair
(fastfields-torch#19, fastfields#21) diagnosed the symptom correctly — the
torch *_add_ ops really were mutating their input — but drew the wrong
conclusion and deleted the Python feature. The right fix is to restore the
missing C primitive
. Those two PRs are superseded and closed.

What jitfields actually does (verified, not assumed)

jitfields/csrc/lib/regularisers/{field,flow}/utils.h:

template <char op, typenamescalar_t, typenamereduce_t = scalar_t>
structOp { staticconstexpr FuncType f = set; };
template <...> structOp<'+', ...> { staticconstexpr FuncType f = iadd; };
template <...> structOp<'-', ...> { staticconstexpr FuncType f = isub; };

Every matvec_* / kernel_* / diag_* entry point takes that char op and
writes through out, so '+'/'-' are read-modify-write on the caller's
buffer
. There is no separate "return a fresh tensor" C entry point.

In jitfields' Python layer the two spellings share that one kernel:

deffield_matvec_add(...): # out-of-placeout=inp.clone() # <- the ONLY differencefn(out, vec, ..., 'add')
deffield_matvec_add_(...): # in-placefn(inp, vec, ..., 'add')

What was actually missing in fastfields

layerstate before
kernelsOp<op> intact
cpu-impl / cuda-implchar op on matvec/kernel/diag — intact
cpu-lib / cuda-libonly {field,flow}_matvec_{add,sub} (task #53); _field_diag/_field_kernel hardcoded '='
lib (hub)nothing — no accumulate entry point at all
bind-py / dlpacknothing
numpy / torch / cupyfaked it in Python: inp + field_matvec(...) (two kernels + a temporary)

So the earlier claim "no accumulate-into-out kernel exists below" was observing
a real gap — but the gap was a missing surface, not a missing capability.

Naming: trailing underscore

The restored entry points are {field,flow}_{matvec,diag,kernel}_{add_,sub_}
with a trailing underscore, because they are in-place only. This matches the
existing ff:: convention for accumulate-into-out (sym_addmatvec_,
sym_submatvec_) and the out-of-place/in-place pairing field_precond /
field_precond_.

This renames the four symbols added by task #53
({field,flow}_matvec_{add,sub} -> ..._{add_,sub_}). The project is
unreleased (0.1+dev), and the rename removes a real Python-level collision:
fastfields.{numpy,torch,cupy}.field_matvec_add is the out-of-place spelling,
so leaving the in-place C primitive under the same name meant one name with
opposite meanings in two importable modules.

This is the one deliberate deviation from "mirror #53 exactly". If you'd
rather keep the un-suffixed spelling, it is a one-line sed to revert.

Python surface (unchanged, and identical to jitfields)

field_matvec_add out-of-place, field_matvec_add_ in-place — on numpy, torch
and cupy alike. Both route through the single in-place C primitive; the
out-of-place form clones first. No behaviour change for existing callers, but
the accumulate is now one fused kernel instead of a matvec plus a temporary.

Divergence from jitfields (deliberate)

jitfields.field_kernel_add calls impl.flow_kernel — an upstream copy-paste
bug (its docstring even reads "See flow_kernel"). fastfields routes
field_kernel_add_ to the field kernel. Not reproduced.

🤖 Generated with Claude Code

Workstream: claude-jitfields-to-fastfields


Generated by Claude Code

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
…_/...)
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
balbasty added a commit that referenced this pull request Aug 1, 2026
…undaries (#54)
* feat(reg_field): reject bending under Replicate/DCT1/DST1 at the dispatch entry
A reach-2 stencil folds +-2 taps and +-1/+-1 corners, and three boundary
conditions are not involutive there:
* Replicate -- clamping is idempotent, not involutive, so at x=0 both
x-1 and x-2 land on 0 and the (0,-2) matrix entry has no (-2,0)
partner to mirror;
* DCT1 / DST1 -- whole-sample symmetry reflects about a different
centre going forward than coming back, so the reflected +-2 tap does
not land where the reverse fold does.
The assembled operator is then not symmetric, which CG and relaxation
have no business running on. Assembling it explicitly and measuring
|A - A^T| / max|A| gives 6%-13% for Replicate and 42%-50% for DCT1;
every other condition (Zero, DCT2, DST2, DFT, NoCheck) measures 0.
Reject those combinations with a thrown std::invalid_argument, ONCE at
the dispatch entry rather than once per voxel -- past this point every
voxel in the stencil loop may assume a self-adjoint-capable boundary with
no runtime branching. `bound::supports_bending` (fastfields-kernels#50)
is the kernels-side predicate, so the rule has a single definition.
`field_kernel` is deliberately exempt: it materialises the interior
Toeplitz stencil at pure strides and never consults the boundary, so a
well-defined answer exists for every condition. Documented at the site.
Also documents `field_diag`'s contract as the EXACT matrix diagonal at
every voxel, boundary voxels included (#50 decision 1), which is what the
new kernels-side engine now computes.
Tests: 40 new checks over all 8 conditions x {matvec, diag, kernel} x
{bending, membrane-only}, asserting both that the three throw with
bending active and that nothing else ever does -- in particular that
reach-1 membrane is accepted under every condition. test_reg_field goes
465 -> 505 checks. Verified the new coverage actually bites by making the
check a no-op and confirming 6 failures, then restoring.
`make test` on clang++ and g++: all 11 suites PASS.
Part of fastfields-kernels#50 and fastfields-kernels#55.
* fixup: correct the rejection set and cover reach-1 (membrane) too
Rework the dispatch-entry check against fastfields-kernels#50's CORRECTED
Decision 2. The first cut of this PR used the original (pre-correction)
set and was wrong in both directions:
* bending + DST1 was rejected and must not be -- DST1's +-2 fold lands
back on the centre voxel (a diagonal entry) and its +-1 fold hits the
sign-0 phantom node, so it is EXACTLY self-adjoint for field bending,
measured 0 at every D;
* membrane + DCT1 was accepted and must not be -- reach-1 energies were
assumed universally self-adjoint (kernels#43) and never checked.
DCT1's whole-sample fold lands the -1 tap of x=0 onto its own +1 tap,
so A[0][1] picks up the fold and A[1][0] does not. Measured 0.25-0.46.
Measured set (assemble `A`, take max|A-A^T|/max|A|):
bound | absolute | membrane | bending
-----------+----------+----------+---------
Replicate | ok | ok | REJECT (0.042-0.13)
DCT1 | ok | REJECT | REJECT (0.25-0.46 / 0.37-0.50)
all others | ok | ok | ok (exactly 0)
So the check is no longer bending-specific. `CHECK_BENDING_BOUND` becomes
`check_selfadjoint_bound(membrane, bending, bnd)`, which mirrors the
wrappers' energy selection EXACTLY -- highest-order non-null penalty wins
-- because a check that disagrees with the kernel it guards is worse than
no check. Still evaluated once per call, never per voxel. `absolute` has
no fold and is accepted under all eight; `field_kernel` stays exempt.
Tests reshaped for the new matrix: 8 bounds x {matvec, diag} x {bending,
membrane, absolute} + field_kernel, plus four assertions pinned
specifically on the two entries that CHANGED, so a regression names the
correction rather than just a row. test_reg_field 505 -> 525.
Both corrections verified to bite by mutation, separately:
* dropping the membrane arm -> 4 failures, all membrane+DCT1
* putting DST1 back in the bending set -> 4 failures, all bending+DST1
then restored and confirmed green.
Verified with GENUINE from-scratch rebuilds, one per compiler (30 real
compile invocations each): all 11 suites PASS on clang++ and on g++.
Earlier "both compilers" claims on this branch were unsound -- `make
clean` does not remove build/testobj or build/test_*, so a g++ run after
a clang run silently re-ran the clang binaries; filed as #55.
Part of fastfields-kernels#50, needs fastfields-kernels#58.
* chore(deps): bump the impl pin so this branch can actually build
The `impl` pin sat at dd85903, whose nested `kernels` pin predates both
halves of fastfields-kernels#50 phase 1 -- so CI, which checks this branch
out with `submodules: recursive`, compiled against a kernels without
`bound::supports_membrane` and failed:
reg_field.cpp: error: no member named 'supports_bending' in namespace
'ff::cpu::bound'
Point it at fastfields-cpu-impl's matching pin bump
(claude/bump-kernels-field-engine), which carries kernels 3096592.
Chain: kernels#58 -> cpu-impl#<pin bump> -> here. Both of those must land
first, and this pin needs re-pointing at cpu-impl's squash-merge commit
when they do.
Part of fastfields-kernels#50.
* chore(deps): re-point impl pin at #41's squash-merge commit
Was pointing at 4e59a4c, the head of cpu-impl#41's now-deleted
branch. Re-pointed at a189b5c, the squash-merge commit on cpu-impl's
teeny branch.
---------
Co-authored-by: Claude <noreply@anthropic.com>
@balbasty
balbasty merged commit f75f3a1 into mainAug 1, 2026
3 checks passed
@balbasty
balbasty deleted the claude/reg-accumulate-inplace-restore branch August 1, 2026 16:50
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