Skip to content

Accept the argnums and switch spellings the signatures promise - #101

Merged
cemde merged 1 commit into
liukidar:mainfrom
cemde:fix/74-signature-mismatches
Aug 11, 2026
Merged

Accept the argnums and switch spellings the signatures promise#101
cemde merged 1 commit into
liukidar:mainfrom
cemde:fix/74-signature-mismatches

Conversation

@cemde

@cemde cemde commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Bug

Two places where the annotation is wider than what the code accepts.

ValueAndGrad is typed argnums: int | Sequence[int], matching jax, but _t splats it: jax.value_and_grad(..., argnums=(*argnums, n)). An int is not iterable.

Switch is typed Sequence[...] and passes fns to _BaseTransform.__init__, which calls _make_tuple(fn). That helper is x if isinstance(x, tuple) else (x,), so a list of branches is wrapped rather than expanded, collapsing all branches into one element.

Impact

Both documented spellings raise:

pxf.value_and_grad(argnums=0)     TypeError: Value after * must be an iterable, not int
pxf.switch([b0, b1, b2])          TypeError: object of type 'function' has no len()

Only the undocumented argnums=(0,) and a tuple of branches worked.

Fix

Normalise at each call site: ValueAndGrad.__init__ wraps an int and tuples anything else, and Switch.__init__ calls super().__init__(tuple(fns)).

Normalising in __init__ rather than _t also fixes a silent bug: a generator argnums worked on the first call and then returned no positional gradient at all on the second, because _t re-splatted the now-exhausted generator.

Why not in _make_tuple

Teaching the shared helper to expand sequences breaks four of its six callers. The decisive one is _process_mask, which passes mask dict keys: str is a Sequence, so "model" would expand into five single-character keys and the mask would fail with a pytree structure error. Two others pass user function return values, where a returned list would be resplit into value plus aux.

Closes #74

`ValueAndGrad` splatted `argnums` into a tuple, so the annotated `int` form
raised `TypeError` and only `(0,)` worked. Normalise it once in the
constructor.

`Switch` passed its `Sequence` of branches straight to `_make_tuple`, which
wraps anything that is not already a tuple, so a list of branches collapsed
into a single callable. Convert to a tuple at the call site rather than
teaching `_make_tuple` to expand sequences, which its other callers rely on
not doing.

Closes liukidar#74
@cemde
cemde force-pushed the fix/74-signature-mismatches branch from 75e43f6 to 5cd6c12 Compare August 9, 2026 18:23
@cemde
cemde requested a review from liukidar August 9, 2026 18:33

@liukidar liukidar left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cemde
cemde merged commit 435d8de into liukidar:main Aug 11, 2026
31 of 33 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.

Transform signatures do not match their implementations (argnums, switch branches)

2 participants