Skip to content

refactor(m2l): the chunked scatter's shared axes -- and the one that is free - #337

Merged
TobiBu merged 3 commits into
mainfrom
refactor/m2l-chunk-scatter-axes
Sep 8, 2026
Merged

refactor(m2l): the chunked scatter's shared axes -- and the one that is free#337
TobiBu merged 3 commits into
mainfrom
refactor/m2l-chunk-scatter-axes

Conversation

@TobiBu

@TobiBu TobiBu commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Second of two PRs from the 2026-09-07 re-recording, and the one that closes
runtime/kernels/_m2l.py out. Companion to #336.

The module is at 1%, and that is the headline

The phase-2 doc had this module at 8% of 286 perturbations and advised skipping it.
Re-recorded on main after cbc99f1: 244 perturbations, 2 accepted — 1%, on 15
measured functions with zero inconclusive and zero unreplayable. The class and accumulator
families are closed, and both survivors are in one function.

Only one of the two is a defect. Separating them is most of this change.

The defect: a gather that JAX clamps

_chunk_segment_scatter_add does

sort_idx        = jnp.argsort(masked_targets)   # length comes from tgt_chunk
contribs_sorted = contribs[sort_idx]            # <-- gathered with THAT length

so a contribs one row short is an out-of-bounds gather, and JAX clamps rather than
raising. Measured on main: the call returned a full (255, 25) accumulator having
silently gathered row 510 twice, scattering one pair's M2L contribution into the wrong
target. chunkflat now ties contribs, tgt_chunk and valid together, and closes it.

The non-defect: a free axis, proven not argued

local_accum's leading axis. 12 recorded calls show it at 7, 15, 31, 127, 255, 511 and
1023 against an unchanged contribs
— it is the target-node count, which has nothing to
do with the chunk width. The pilot was perturbing an axis bound by one parameter with
nothing deriving from it, which produces a well-formed call.

So test_the_accumulators_node_axis_stays_free asserts it stays accepted. Without that,
the pilot's own report is a standing invitation to cross-bind an axis the evidence
contradicts.

Evidence for the two axes that are real

Read per call, 12 recordings, 9 distinct combinations:

axis evidence
chunkflat contribs leading == tgt_chunk == valid in all 9, at two extents (512, 4096)
sh contribs trailing == local_accum trailing in all 9, at four extents (4, 9, 25, 81)

sh was already rejected on main by broadcasting, so its test accepts either exception
and its docstring says so: the annotation pins the name, it does not close a hole.

Inexact, not Float

The recording carries complex128, complex64 and float64 through both contribs
and local_accum. Narrowing to Float would reject the complex basis outright — the
mistake #293 shipped one module over, from a real-basis-only recording, for 27 CI failures.
A parametrised control test now covers both bases.

The doc commit

The second commit rewrites the phase-2 document's rows for all three modules in this pass
(30/29/8% → 7/8/1%) and names two categories it did not have:

  • the Pallas clamp — 9 of the pass's 20 acceptances (a third commit corrects this: I
    first wrote 17, which was 20 minus the 3 free axes, a different quantity). A pallas_call
    takes its grid from one array's shape and indexes every other operand with the same block
    index, so a short operand is clamped rather than rejected. The pure-JAX twins reject the identical
    perturbation. nearfield_fused_leaf_jax accepted 0 of 21 where its _pallas twin
    accepted 4, which makes the Pallas lane strictly weaker at shape validation than the lane
    _m2l.py's docstring requires it to equal;
  • the free axis — the category above, 3 of the 20, so future passes stop counting them
    as backlog.

It also records the prediction tally, 4 right / 4 wrong / 1 partial, because the misses
share a direction: I predicted acceptance from "nothing annotates it" and forgot that
ordinary array arithmetic rejects most shape errors by failing to broadcast. The four
correct predictions were all about broadcast reductions, where it does not.

Tests

Appended to tests/unit/runtime/test_m2l_shape_contracts.py5 tests, 1 red against
main
. That ratio is the honest one for a module at 1%; the other four are two controls and
two deliberate pins, each labelled as green-on-main in its own docstring.

Verification

pre-commit run --all-files                        all pass
JAX_ENABLE_X64=1 pytest                           1918 passed, 137 skipped
JAX_ENABLE_X64=1 pytest tests/characterization     34 passed  (golden unmoved)
JACCPOT_RUNTIME_TYPECHECK=1 pytest tests/unit/test_type_annotation_guard.py   5 passed

Census — bench/annotation_census.py, quoted not retyped:

main    844 shaped / 1253 bare   40.2%      65 bare / 27 shaped  runtime/kernels/_m2l.py
branch  848 shaped / 1249 bare   40.4%      61 bare / 31 shaped  runtime/kernels/_m2l.py

Note on the .pre-commit-config.yaml conflict with #336

Both PRs add to the flake8 --builtins list: this one chunkflat, #336 rows,cols.
Whichever merges second wants the union of the two, which is the same resolution used in
this programme twice already.

🤖 Generated with Claude Code

TobiBu and others added 2 commits September 7, 2026 20:26
…is free

The 2026-09-07 re-recording puts this module at 2 silent acceptances of 244
perturbations -- 1%, down from the 8% on 286 that the doc's advice ("last, and
possibly not worth a PR") was written against. cbc99f1 closed the class and
accumulator families, so both remaining acceptances are in one function.

Only ONE of the two is a defect, and separating them is most of this change.

THE DEFECT. `_chunk_segment_scatter_add` does `contribs[sort_idx]` with a
`sort_idx` whose length comes from `tgt_chunk`, so a `contribs` one row short is an
out-of-bounds gather -- and JAX CLAMPS rather than raising. Measured on main: the
call returned a full (255, 25) accumulator having silently gathered row 510 twice,
scattering one pair's contribution into the wrong target. `chunkflat` now ties
`contribs`, `tgt_chunk` and `valid` together and closes it.

THE NON-DEFECT. `local_accum`'s leading axis. 12 recorded calls show it at 7, 15,
31, 127, 255, 511 and 1023 against an unchanged `contribs` -- it is the target-node
count, which has nothing to do with the chunk. The pilot was perturbing a free
axis, and a test now asserts it STAYS accepted so the report cannot talk someone
into cross-binding it later.

`sh` is shared between `local_accum` and `contribs` in all 9 distinct recorded
combinations, at 4, 9, 25 and 81. That one was already rejected on main by
broadcasting, so its test accepts either exception and says so: the annotation
pins the name, it does not close a hole.

`Inexact` and not `Float`. The recording carries complex128, complex64 AND float64
through `contribs` and `local_accum`, so narrowing to `Float` would reject the
complex basis outright -- the mistake #293 shipped one module over.

Five tests, one red against main. That ratio is the honest one for a module at 1%.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…egories

The phase-2 document's rows for these three were all measured BEFORE the PRs that
annotated them, which is the same staleness it was already caught by twice. 30%,
29% and 8% are actually 7%, 8% and 1%: 474 perturbations, 20 accepted.

Two things the document did not have a name for, both now written down.

THE PALLAS CLAMP. 17 of the 20 acceptances are in Pallas entry points and every one
is an extent mismatch, because a `pallas_call` takes its grid from ONE array's shape
and indexes every other operand with the same block index -- so a short operand is
read out of bounds and JAX CLAMPS. The pure-JAX twins reject the identical
perturbation by failing to broadcast. `nearfield_fused_leaf_jax` accepted 0 of 21
while its `_pallas` twin accepted 4, which makes the Pallas lane strictly weaker at
shape validation than the lane it is required to equal.

THE FREE AXIS. Where an axis is bound by exactly one parameter and nothing derives
from it, perturbing it yields a well-formed call. The pilot counts that as silently
accepted -- correct by its own definition, but not work. 3 of the 20 are this, proven
on `local_accum`, whose leading axis the recording shows at seven different values
against an unchanged `contribs`.

Also records the prediction tally, 4 right / 4 wrong / 1 partial, because the misses
share a direction: predicting acceptance from "nothing annotates it" and forgetting
that ordinary arithmetic rejects most shape errors on its own. The four correct
predictions were all about BROADCAST reductions, where it does not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@TobiBu
TobiBu force-pushed the refactor/m2l-chunk-scatter-axes branch from 31fdeb3 to 4b76188 Compare September 7, 2026 18:26
Two fixes to the section added a commit ago, both mine.

THE COUNT WAS WRONG. "17 of the 20 acceptances are in Pallas entry points" is 9.
The 17 was 20 minus the 3 free axes -- a different quantity, "acceptances that are
arguably real defects" -- and I wrote one number while meaning the other. Counted
from the replay: 9 Pallas entry points, 8 broadcast helpers, 2 the chunked scatter,
1 a pure-JAX twin. The section now lists the split instead of asserting a total.

THE MECHANISM CLAIM WAS TOO BROAD. "every one is a leading- or trailing-extent
mismatch" does not describe the helper group, where four of the eight are RANK
changes -- an extra leading axis or a flattened operand -- which change which axis
is reduced rather than which extent. There are two mechanisms, not one: the Pallas
clamp for the entry points, the broadcast for the helpers. Both are now stated
separately, and the second explains why the eight sit in the three helpers that
reduce by hand and not in the nine matmul functions that accepted zero.

Also records the decision on the remaining 9: strengthen the body guard rather than
annotate, so the documented `ValueError` stays reachable and stays what callers see.
The general rule is written down for the next module that hits it -- where a
parameter's own body documents a ValueError over its shape, an annotation is the
wrong instrument even when it would close something real.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@TobiBu
TobiBu merged commit 1435c92 into main Sep 8, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant