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

pushpull: route bound/spline dispatch through static/Dynamic selector macros + add SPLINEFLAGS - #48

Merged
balbasty merged 6 commits into
mainfrom
claude/cuda-pushpull-dynamic-spline
Jul 31, 2026
Merged

pushpull: route bound/spline dispatch through static/Dynamic selector macros + add SPLINEFLAGS#48
balbasty merged 6 commits into
mainfrom
claude/cuda-pushpull-dynamic-spline

Conversation

@balbasty

Copy link
Copy Markdown
Contributor

Workstream: claude-jitfields-to-fastfields

Part of fastfields/fastfields-cuda-lib#30 (wire pushpull into cuda-lib's CI). This is the cpu-lib layer piece. Default policy is unchanged (everything static) — plumbing to let a different build policy (which cuda-lib now uses by default) actually take effect.

  • Dispatch macros (PP_BOUND/PP_ORDER, both the full and FF_TEST_SPARSE variants) now instantiate FF_BOUND_<NAME>/FF_SPLINE_<NAME> instead of bound::type::<NAME>/spline::type::<NAME> directly, so a Dynamic build policy (BOUNDFLAGS/SPLINEFLAGS) routes the flagged conditions/orders through the shared bound::type::Dynamic/spline::type::Dynamic instantiation instead of a dedicated one — mirrors how reg_field.cpp/reg_flow.cpp already use FF_BOUND_<NAME> (reg_field/reg_flow: select static-or-Dynamic bounds per build (BOUNDFLAGS) #46).
  • pull/push/count/grad build a bound::BoundVec + spline::SplineVec from the runtime bound/spline arguments and forward them through the leaf wrappers into the impl launchers, so whichever axis ends up Dynamic can recover its actual condition/order at run time.
  • Added SPLINEFLAGS (default empty = all 8 orders static, unchanged behaviour), the spline-order analogue of BOUNDFLAGS, wired into both the library and test compile rules.

Depends on fastfields/fastfields-cpu-impl#38 (bumped the impl pin to it), which in turn depends on fastfields/fastfields-kernels#44.

Test plan

  • test_pushpull: 308/308 under the default (unchanged) all-static policy.
  • test_pushpull: 308/308 under -DFF_STATIC_SPLINES=0 -DFF_STATIC_SPLINE_{NEAREST,LINEAR,QUADRATIC,CUBIC}=1 -DFF_STATIC_BOUNDS=0 -DFF_STATIC_BOUND_{DCT2,DST2}=1 — the exact policy fastfields-cuda-lib now ships by default. This run originally caught two pre-existing kernels-layer bugs (fixed in pushpull: implement spline::type::Dynamic + fix two Dynamic-order/-bound bugs fastfields-kernels#44); 308/308 after the fix.
  • test_pushpull: 308/308 under a fully-Dynamic policy (-DFF_STATIC_SPLINES=0 -DFF_STATIC_BOUNDS=0).

🤖 Generated with Claude Code


Generated by Claude Code

claude added 3 commits July 30, 2026 22:10
…ctor macros
- Dispatch macros (PP_BOUND/PP_ORDER, both the full and FF_TEST_SPARSE
variants) now instantiate FF_BOUND_<NAME>/FF_SPLINE_<NAME> instead of
bound::type::<NAME>/spline::type::<NAME> directly, so a Dynamic build
policy (BOUNDFLAGS/SPLINEFLAGS) routes the flagged conditions/orders
through the shared bound::type::Dynamic / spline::type::Dynamic
instantiation instead of a dedicated one -- mirrors how reg_field.cpp/
reg_flow.cpp already use FF_BOUND_<NAME> (#46).
- pull/push/count/grad build a bound::BoundVec + spline::SplineVec from
the runtime bound/spline arguments and forward them through the leaf
wrappers into the impl launchers, so whichever axis ends up Dynamic
can recover its actual condition/order at run time.
- Added SPLINEFLAGS (default empty = all 8 orders static, unchanged
behaviour), the spline-order analogue of BOUNDFLAGS, wired into both
the library and test compile rules.
The CPU backend has no ptxas memory problem, so this is plumbing only:
default policy is unchanged (everything static), verified by the full
pushpull oracle suite (test_pushpull: 308/308) after rebuilding with the
updated fastfields-cpu-impl/fastfields-kernels pins.
Part of fastfields-cuda-lib#30.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
Part of fastfields-cuda-lib#30.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
The prior commit (1d80f94) intended to bump the `impl` submodule pin via
the safe procedure (`git update-index --add --cacheinfo 160000,<SHA>,
impl`), but the final `git commit -- impl` step re-staged the path from
the working tree before committing -- and the working tree holds a
*symlink* (this dev tree's local convention), not the submodule gitlink.
That silently replaced the staged 160000 gitlink with a 120000 symlink
blob, corrupting the pin. `git ls-tree` on that commit confirmed `120000
blob ... impl` instead of `160000 commit ...`.
Re-applies the same procedure and commits with a bare `git commit` (no
trailing pathspec) so the already-staged gitlink is used as-is. Now points
at the cpu-impl commit with its own corrected `kernels` gitlink, which in
turn points at the kernels commit with the two pushpull Dynamic-order/
-bound correctness fixes.
Part of fastfields-cuda-lib#30.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
@balbasty

Copy link
Copy Markdown
ContributorAuthor

Looks good, with one question:

Can we get rid of the bound/order sparse macros and fold everything using the per-bound/order defines? I.e in the make file, have a sparse (or test) target that uses a different set of bound/order defines than the release tag?

claude added 2 commits July 31, 2026 14:10
…LINE_* policy
Addresses review feedback on #48: two parallel mechanisms were trimming
pushpull's test-compile matrix -- the hand-duplicated `-DFF_TEST_SPARSE`
switch (PP_BOUND_SPARSE/the alternate PP_ORDER), which hard-coded a
*covering* subset of the order x bound matrix and literally threw on
anything outside it, and the newer FF_BOUND_<NAME>/FF_SPLINE_<NAME>
Dynamic-routing policy. The latter already achieves the former's actual
goal (fewer static instantiations -> faster compile) without the
correctness cost: every bound/order combination stays fully functional
(via the shared Dynamic instantiation) rather than throwing outside the
hard-coded covering set.
- pushpull.cpp: removed the `#ifdef FF_TEST_SPARSE` branch entirely.
There is now exactly one PP_ORDER/PP_BOUND, always routed through
FF_BOUND_<NAME>/FF_SPLINE_<NAME>; "sparse vs. full" is purely a
BOUNDFLAGS/SPLINEFLAGS build-time choice, not a code-level branch.
(`-DFF_TEST_SPARSE` is left defined for the `test` target -- resize.cpp/
restrict.cpp have their own, unrelated use of it, out of scope here --
but pushpull.cpp no longer reacts to it.)
- Makefile: the `test` target gets its own *default* BOUNDFLAGS/SPLINEFLAGS
(the same mixed policy fastfields-cuda-lib ships) via a target-specific
variable, so a bare `make test` stays fast the way `-DFF_TEST_SPARSE`
used to keep it, without a separate mechanism. An explicit override
(`make test BOUNDFLAGS=... SPLINEFLAGS=...`, as CI's matrix does) still
wins -- command-line-origin variables outrank any in-makefile
assignment. (Note: target-specific values must use plain `=`, not `?=`
-- the top-level `BOUNDFLAGS ?=`/`SPLINEFLAGS ?=` already "set" them at
parse time, so a target-specific `?=` would never fire.)
- CI workflow: each of the 3 matrix legs (static/dynamic/cuda-default) now
pins SPLINEFLAGS explicitly alongside BOUNDFLAGS, so a leg's name
describes its policy on both axes -- without this, "static"/"dynamic"
would have silently inherited the `test` target's new sparser default
on whichever axis they didn't already override, weakening their
coverage guarantee.
Verified locally: full `make test` suite (11 binaries) green under the
new bare-`make test` default; test_pushpull individually green (308/308)
under all three CI-matrix policies (static, dynamic, cuda-default).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
Part of fastfields-cuda-lib#30.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
@balbastyClaude

Copy link
Copy Markdown
ContributorAuthor

Addressed the feedback on folding FF_TEST_SPARSE into the per-bound/order defines (pushed to claude/cuda-pushpull-dynamic-spline):

  • Removed the #ifdef FF_TEST_SPARSE ... #else ... #endif duplicate PP_ORDER/PP_BOUND dispatch-macro block in pushpull.cpp entirely — there is now exactly one unconditional set of macros, always built from FF_SPLINE_<NAME>/FF_BOUND_<NAME> (the same per-bound/order define mechanism BOUNDFLAGS/SPLINEFLAGS already drive).
  • Instead of the sparse macro, the test Makefile target now sets its own defaults for BOUNDFLAGS/SPLINEFLAGS (target-specific variables, e.g. test: BOUNDFLAGS = -DFF_STATIC_BOUNDS=0 -DFF_STATIC_BOUND_DCT2=1 -DFF_STATIC_BOUND_DST2=1) distinct from the top-level release default, while still yielding to a command-line override (make test BOUNDFLAGS=...). CI's 3 matrix legs (static/dynamic/cuda-default) now pass explicit BOUNDFLAGS/SPLINEFLAGS for both axes.
  • -DFF_TEST_SPARSE itself is left alone in TESTCPPFLAGS where resize.cpp/restrict.cpp still use it independently — out of scope for this fold, confirmed via grep that pushpull no longer references it at all.
  • Verified: full test suite (11 binaries) passes under all 3 policies (308/308).

Checked fastfields-cuda-lib for an analogous sparse-macro/test-target to fold — it has no FF_TEST_SPARSE and no test target at all, so there's nothing to consolidate on that side.

(GitHub's GraphQL review-thread API is rate-limited for this account right now, so I'm leaving this as a regular comment rather than an inline reply.)


Generated by Claude Code

…ing merge)
cpu-impl#38 merged as 9e12e7d (merge commit SHA differs from the PR
branch head this was previously pinned to).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
@balbasty
balbasty merged commit 3cf1004 into mainJul 31, 2026
3 checks passed
@balbasty
balbasty deleted the claude/cuda-pushpull-dynamic-spline branch July 31, 2026 21:50
balbasty added a commit that referenced this pull request Aug 3, 2026
…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>
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