Make the PyTorch backend work, and document JAX/PyTorch as selectable backends - #7
Open
geekbeast wants to merge 5 commits into
Open
Make the PyTorch backend work, and document JAX/PyTorch as selectable backends#7geekbeast wants to merge 5 commits into
geekbeast wants to merge 5 commits into
Conversation
…AX backend
The diagonal sweep keyed s_start/t_start off `cols`, which walks s past the
bottom edge of the grid whenever rows != cols. Anti-diagonal d covers the cells
{(s, t) : s + t == d}; s_start is the largest such s and t_start the smallest
such t, so s must pin at rows - 1, not cols - 1. Checked against a brute-force
enumeration of the grid, the old convention was wrong for 110 of the (d, rows,
cols) triples with rows, cols <= 6, and every one of them had rows != cols.
The effect was a silently wrong signature kernel for any pair of paths sampled
at different lengths. The kernel depends on the paths, not on how finely they
are sampled, so upsampling a piecewise-linear path along its own segments must
leave the value unchanged; for a 6-point and a 5-point path regridded to a
common 21 points, the kernel was off by 2.7e-2 and the error did not shrink
with truncation order.
Separately, compute_signature_kernel is jitted but assigned to self.exponents,
which stores a tracer on the instance and leaks it into every later call -- a
compute_signature_kernel followed by a compute_gram_matrix on the same instance
died with InvalidInputException. self.exponents is already placed in __init__.
Both fixes come with regression tests; all five fail on the previous code.
…t in CI The PyTorch backend on main had never run. PowerSigTorch had no __call__; compute_signature_kernel passed a dtype into build_stencil_s's device parameter and died in dynamo on torch.ones(..., device=torch.float64); compute_gram_matrix was a copy-paste of the JAX version, calling jnp.zeros (never imported) and .at[0].set(1) on a torch tensor; and compute_gram_entry returned from inside its outer loop after the first chunk, with debug prints still in place. None of this was caught because there were no PyTorch tests and CI ran only tests/test_core_jax.py. This takes the implementation from feature/custom-autodiff, which reaches API parity with the JAX backend -- __call__, compute_gram_matrix, pluggable static kernels, device/dtype selection, block sizing, and autodiff -- along with its three test modules. Verified against the JAX backend and against closed form: a linear path matches I_0(2) to 1e-12, Gram matrices agree with JAX to 5e-15 across linear and RBF static kernels, CUDA agrees with CPU to 4e-15, and reverse-mode gradients match central differences to 1.4e-9 relative. CI now runs a backend matrix so neither implementation can rot silently again. The torch job installs the JAX CPU wheel too, since the PyTorch tests cross-check their results against the JAX reference.
The README only showed the JAX path, so there was no way to tell that a PyTorch backend existed or how to install it. Install now leads with the per-backend extras, Getting Started carries a runnable quickstart for each, and a "Choosing a backend" section maps the two APIs onto each other and says when to reach for which. Adds examples/simple_torch.py as the PyTorch mirror of examples/simple.py. Install commands switched to the published PyPI package with extras; the previous plain git URL installed no backend at all, and the #egg= form does not carry extras. CuPy is described as forward-Gram only, which is what powersig.cupy_backend actually exposes.
The sweep runs this arithmetic once per anti-diagonal inside the hot loop, so a correctness fix that reached for jnp.where, a Python-level branch, or a helper call would pay for itself there. The geometry correction on feature/custom-autodiff took that route -- it swapped the inline arithmetic for a get_diagonal_range call and added a boolean-arithmetic variant plus timing coverage to compare the two. This fix keeps the inline boolean form and only corrects the indices, so the operation count is unchanged. Measured on the compiled module for a 129-point pair at order 8, main and this branch come out at 245819 and 245811 flops; the 8-flop delta is the tracer-leak fix, and isolating the geometry change alone reproduces main's 245819 exactly. The test compares the corrected expression against the one it replaced, compiling both in-process with the same JAX build rather than asserting a recorded number, so it does not drift as JAX changes its cost model. A second test pins the expression to powersig/util/grid.py so the copy in the test cannot silently diverge from the shipped geometry.
compile_forward=True routes the sweep through torch.compile with
mode="reduce-overhead", which replays a CUDA graph writing into a fixed output
buffer. compute_signature_kernel returned a view of that buffer, so a result the
caller was still holding got silently overwritten by their next call:
ps = PowerSigTorch(order=8, compile_forward=True)
ks = [ps.compute_signature_kernel(x, y) for x, y in pairs]
# every entry of ks held the LAST pair's value
Values were correct when consumed immediately, which is why timing code never
trips over this -- a benchmark reads the number straight away. Measured drift on
held tensors was 1.49 absolute before this change and 0.0 after.
compute_gram_matrix was never affected: it consumes each entry into the output
matrix before the next call. Confirmed at 2.7e-15 against the eager path,
symmetric, every entry matching an independent pairwise computation.
The fix clones the scalar out of the graph buffer before returning it. The
output is a scalar, so the copy costs nothing next to the sweep, and the CPU
path is untouched since compile_forward only applies on CUDA.
Also documents the flag in the README, because leaving it off is expensive:
eager, the sweep costs a flat ~257us per anti-diagonal whatever the truncation
order -- it is bound by kernel-launch overhead, not arithmetic, so order 8 and
order 32 run at the same speed. On an RTX 4090 at 129 points, order 8, that is
65.7ms eager vs 5.6ms compiled vs 6.4ms for JAX. The flag is not free to enable
(CUDA only, and dynamic=False means a compile pause of tens of seconds per input
shape), which is presumably why it defaults to off.
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 free
to 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.
What this is
The ask was to check whether the PyTorch implementation works and, if so, document JAX and PyTorch as selectable backends. It did not work — on
mainit had never run — so this makes it work first, then documents both.The PyTorch backend on
mainwas never functionalPowerSigTorchhad no__call__, so it could not be used likePowerSigJax.compute_signature_kernelpassed a dtype intobuild_stencil_s'sdeviceparameter and died in dynamo ontorch.ones(..., device=torch.float64).compute_gram_matrixwas a copy-paste of the JAX version: it calledjnp.zeros(never imported) and.at[0].set(1)on a torch tensor.compute_gram_entryreturned from inside its outer loop after the first chunk, with debugprints still in place.None of this was caught because there were no PyTorch tests and CI ran only
tests/test_core_jax.py.This takes the implementation from
feature/custom-autodiff, which reaches API parity with the JAX backend, plus its three test modules. Onlypowersig/torch/*is taken — the branch's JAX rewrite and its pre-CUDA-13pyproject.tomlare deliberately left behind.Two real JAX bugs surfaced on the way
Porting the torch tests, which cross-check against JAX, turned up two pre-existing defects in the JAX backend. Both are fixed here, minimally.
1. The diagonal sweep was wrong whenever the two paths had different lengths. The geometry keyed
s_start/t_startoffcols, which walksspast the bottom edge of the grid whenrows != cols. Against a brute-force enumeration, the old convention was wrong for 110 of the(d, rows, cols)triples withrows, cols <= 6— every one withrows != cols.The signature kernel depends on the paths, not on how finely they are sampled, so upsampling a piecewise-linear path along its own segments must leave the value unchanged. For a 6-point and a 5-point path regridded to a common 21 points:
The error did not shrink with truncation order, confirming geometry rather than truncation.
2. A jit tracer leak.
compute_signature_kernelis jitted but assigned toself.exponents, storing a tracer on the instance. Acompute_signature_kernelfollowed by acompute_gram_matrixon the same instance died withInvalidInputException.Both come with regression tests. All five new JAX tests fail on the previous code and pass here.
Verification
The ported backend was checked independently of its own test suite:
Also in here
[jax, torch]backend matrix, so neither implementation can rot silently again. The torch job installs the JAX CPU wheel too, since the PyTorch tests cross-check against the JAX reference.examples/simple_torch.py, the PyTorch mirror ofexamples/simple.py. Both were run end to end.Note
powersig/cupy_backendalso consumes the correctedget_diagonal_range, so its asymmetric-input behaviour changes too — in the same direction. It has no test coverage and CuPy was not exercised here.