FIX(pushpull): implement the N-dimensional backward passes - #6
Open
balbasty wants to merge 1 commit into
Open
Conversation
`pushpull/nd.h` was a stub: the backward passes were copy-pasted from the 3D kernels (they used the `x/y/z` argument convention, referenced undeclared variables, and hard-coded three coordinate gradients), and the forward passes did not compile either. Nothing included the file, so none of it was ever built. Rewrite it as a genuine N-dimensional implementation: * `PushPullND<D, ABS>` is now a class template of its own instead of a partial specialization of `PushPull`. `PushPull` only carries three (spline order, boundary condition) pairs, so for D > 3 they must be passed at runtime -- and a `PushPull<D, Z, B0, ...>` specialization would have been ambiguous with the 3D one for D == 3. * All nine kernels (pull / push / count / grad / hess and the pull / push / count / grad backward passes) enumerate the support of the separable basis over D dimensions, with per-dimension spline order and boundary condition read from the `inter` / `bnd` arrays (same convention as `resize.h`). * The coordinate gradients are accumulated over all D dimensions, and `grad_backward` builds the full D x D symmetric matrix of second derivatives instead of the six 3D components. * `hess` writes the compact symmetric layout used elsewhere in jitfields: the diagonal first, then the upper triangle in row-major order (`[xx, yy, zz, xy, xz, yz]` in 3D). * `pushpull.h` now includes `nd.h`, so the file is actually compiled. Add `csrc/tests/test_pushpull_nd.cpp`, which checks every kernel for D = 1..6 against an independent reference that enumerates the basis explicitly and uses its own B-spline weight/derivative formulas, and cross-checks the D == 3 results against the specialized 3D kernels. `tests/test_pushpull_nd.py` compiles and runs it as part of the pytest suite (skipped if no C++ compiler is available). Closes#4 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XQTnUfyLAVMnY1xvRya8U1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#4
What was wrong
jitfields/csrc/lib/pushpull/nd.hwas a stub. As reported in #4, the backward passes were copy-pasted verbatim from the 3D kernels: they still took thex, nx, sx, y, ny, sy, z, nz, szargument convention, calledutils_x/y/z::gindex, and wrote exactly three coordinate gradients (gout[0],gout[osg],gout[osg*2]) regardless ofD.The forward passes were in no better shape — they did not compile at all:
sd = s + 8*dwheresis never declared (the array is calledf),db[d]used as the loop bound where the array is calledl,coordreferenced inpush/count/grad, which don't have such a parameter,*out = static_cast<scalar_t>(acc)ingrad, whereaccis an array,grad:acc[dd] += val * (g[8*d + ] * weights[D-1];,#endif JF_PUSHPULL_ND(trailing tokens after#endif).None of this was ever caught because nothing included the file:
lib/pushpull.honly pulled in1d.h,2d.hand3d.h.What this PR does
nd.his rewritten as a genuine N-dimensional implementation.PushPullND<D, ABS>is now a class template of its own rather than a partial specialization ofPushPull. Two reasons:PushPullonly carries three(spline order, boundary condition)pairs, so forD > 3they have to be runtime parameters anyway — they are read frominter[]/bnd[]arrays, exactly likeresize::Multiscale;PushPull<D, Z, B0, Z, B0, Z, B0, ABS>would have been ambiguous with the 3D specializationPushPull<three, Z, BX, Z, BY, Z, BZ, ABS>as soon as the header was actually included.PushPullNDwas already the name used to reach the fallback (it was an alias inpushpull/utils.h, unused anywhere), so call sites are unaffected.All nine kernels —
pull,push,count,grad,hess,pull_backward,push_backward,count_backward,grad_backward— now enumerate the support of the separable basis overDdimensions. Spatial derivatives are obtained by swapping the weight of the differentiated dimension for its derivative:Ddimensions;grad_backwardbuilds the fullD x Dsymmetric matrix of second derivatives (d2/dx_d dx_e) instead of the six hard-coded 3D components, and contracts it with the incoming gradient;hesswrites the compact symmetric layout used elsewhere in jitfields — the diagonal first, then the upper triangle in row-major order, i.e.[xx, yy, zz, xy, xz, yz]in 3D.lib/pushpull.hnow includesnd.h, so the file is compiled from here on.Tests
jitfields/csrc/tests/test_pushpull_nd.cpp(new) checks every kernel against an independent reference: it enumerates the support explicitly and uses its own B-spline weight / first-derivative / second-derivative formulas, so the kernels are not validated against the very functions they call.Coverage:
D = 1..6, orders 0-3, boundary conditionsdct1 / dct2 / dst2 / dft / replicate, mixed order+boundary across dimensions, several channels, non-cubic shapes, and theABS=truevariant. On top of that, theD == 3results are cross-checked against the specialized 3D kernels.jitfields/tests/test_pushpull_nd.py(new) compiles and runs it as part of the normal pytest suite; it skips itself if no C++ compiler is on the machine. It does not import torch or cppyy, so it runs anywhere.Verified with both
g++ 13andclang++. A mutation check (makingnode_gradalways differentiate along dimension 0) turns 51 of the 141 checks red, so the suite does bite.The existing
cpp/pushpull.hpptranslation unit was compiled before and after the change to confirm that includingnd.hbreaks nothing on the existing paths.Two things deliberately left out of this PR
The N-D path is still not reachable from python.
bindings/{cpp,cuda}/pushpull.pydispatch tojf::pushpull::pullnd,pushnd,countnd,gradnd,hessnd,pullnd_backward, ... whenndim > 3, but those loop wrappers do not exist incsrc/cpp/pushpull.hpp/csrc/cuda/pushpull.cu(and the template argument lists the bindings build for them do not match the ones the 1D/2D/3D wrappers use). Wiring that up — plus the CUDA side — is a bigger, separate piece of work; this PR fixes and tests the kernels themselves, which is what Fix pushpull backward when D > 3 #4 asks for. Happy to open a follow-up issue for the plumbing.Two pre-existing bugs in
3d.h, spotted while cross-checking, left untouched so as not to bundle unrelated changes:hess,accxzis written to both thezzand thexzslot, soacczzis computed and then dropped — this is whyhessis not part of the N-D vs 3D cross-check;grad_backwardreturns a zero coordinate gradient. The diagonal second derivatives of a multilinear basis are indeed zero, but the mixed ones (d2/dx dy) are not, so the term is not identically zero. The N-D kernel computes them; the cross-check for the linear case therefore skipsgrad_backward'sgout(flagged with a comment in the test).🤖 Generated with Claude Code
https://claude.ai/code/session_01XQTnUfyLAVMnY1xvRya8U1
Generated by Claude Code