Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/jax_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ jobs:
0.8.3,
0.9.1,
0.9.2,
0.10.0,
0.10.1,
0.10.2,
]
# 0.7.0 have performance issues but we still support it, see diffrax#680
# 0.7.1 fails with equinox, see equinox#1081
Expand Down
43 changes: 29 additions & 14 deletions desc/batching.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
_check_output_dtype_jacrev,
_jacfwd_unravel,
_jacrev_unravel,
_jvp,
_std_basis,
_vjp,
)
from jax._src.api_util import _ensure_index, argnums_partial, check_callable
from jax._src.numpy.vectorize import (
Expand Down Expand Up @@ -409,6 +407,27 @@
return wrapped


def _argnums_partial(fun, argnums, args, kwargs):
"""Bind every argument except those in ``argnums``.

This mirrors JAX's internal ``argnums_partial`` but returns a plain callable
(and the tuple of differentiated arguments) instead of a
``linear_util.WrappedFun``. A plain callable is what the public
``jax.jvp``/``jax.vjp`` expect, so we avoid the private ``jax._src`` helpers
(e.g. ``_jvp``, which was removed in JAX 0.10.2).
"""
argnums_t = (argnums,) if isinstance(argnums, int) else tuple(argnums)
dyn_args = tuple(args[i] for i in argnums_t)

def f_partial(*dyn):
full_args = list(args)
for i, a in zip(argnums_t, dyn):
full_args[i] = a
return fun(*full_args, **kwargs)

return f_partial, dyn_args


def jacfwd_chunked(
fun,
argnums=0,
Expand Down Expand Up @@ -463,18 +482,17 @@

@wraps(fun, docstr=docstr, argnums=argnums)
def jacfun(*args, **kwargs):
f = lu.wrap_init(fun, kwargs)
f_partial, dyn_args = argnums_partial(
f, argnums, args, require_static_args_hashable=False
)
f_partial, dyn_args = _argnums_partial(fun, argnums, args, kwargs)
tree_map(partial(_check_input_dtype_jacfwd, holomorphic), dyn_args)
if not has_aux:
pushfwd = partial(_jvp, f_partial, dyn_args)
pushfwd = lambda tangents: jax.jvp(f_partial, dyn_args, tangents)
y, jac = vmap_chunked(pushfwd, chunk_size=chunk_size)(_std_basis(dyn_args))
y = tree_map(lambda x: x[0], y)
jac = tree_map(lambda x: jnp.moveaxis(x, 0, -1), jac)
else:
pushfwd = partial(_jvp, f_partial, dyn_args, has_aux=True)
pushfwd = lambda tangents: jax.jvp(

Check warning on line 493 in desc/batching.py

View check run for this annotation

Codecov / codecov/patch

desc/batching.py#L493

Added line #L493 was not covered by tests
f_partial, dyn_args, tangents, has_aux=True
)
y, jac, aux = vmap_chunked(pushfwd, chunk_size=chunk_size)(
_std_basis(dyn_args)
)
Expand Down Expand Up @@ -550,15 +568,12 @@

@wraps(fun, docstr=docstr, argnums=argnums)
def jacfun(*args, **kwargs):
f = lu.wrap_init(fun, kwargs)
f_partial, dyn_args = argnums_partial(
f, argnums, args, require_static_args_hashable=False
)
f_partial, dyn_args = _argnums_partial(fun, argnums, args, kwargs)
tree_map(partial(_check_input_dtype_jacrev, holomorphic, allow_int), dyn_args)
if not has_aux:
y, pullback = _vjp(f_partial, *dyn_args)
y, pullback = jax.vjp(f_partial, *dyn_args)
else:
y, pullback, aux = _vjp(f_partial, *dyn_args, has_aux=True)
y, pullback, aux = jax.vjp(f_partial, *dyn_args, has_aux=True)

Check warning on line 576 in desc/batching.py

View check run for this annotation

Codecov / codecov/patch

desc/batching.py#L576

Added line #L576 was not covered by tests
tree_map(partial(_check_output_dtype_jacrev, holomorphic), y)
jac = vmap_chunked(pullback, chunk_size=chunk_size)(_std_basis(y))
jac = jac[0] if isinstance(argnums, int) else jac
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
jax >= 0.6.2, != 0.7.1, < 0.10.0
jax >= 0.6.2, != 0.7.1, <= 0.10.1
colorama <= 0.4.6
diffrax >= 0.6.0, <= 0.7.2
equinox >=0.11.10, <=0.13.8
Expand Down
9 changes: 8 additions & 1 deletion tests/test_examples.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,14 @@ def test_solve_bounds():
obj = ObjectiveFunction(
ForceBalance(normalize=False, normalize_target=False, bounds=(-3e3, 3e3), eq=eq)
)
eq.solve(objective=obj, ftol=1e-16, xtol=1e-16, maxiter=200, verbose=3)
eq.solve(
objective=obj,
ftol=1e-16,
xtol=1e-16,
maxiter=200,
verbose=3,
options={"tr_method": "svd"},

@YigitElma YigitElma Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consider explaining this issue in tr_method docs for QR. If there is a chance that the jacobian is singular, then SVD is the more reliable option. If the problem is well-posed otherwise, having a wide bounds can cause this (some of the columns are completely 0).

)

# check that all errors are nearly 0, since residual values are within target bounds
f = obj.compute_scaled_error(obj.x(eq))
Expand Down
1 change: 1 addition & 0 deletions tests/test_integrals.py
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,7 @@ def filter(z1, z2):
return z1[mask], z2[mask]

@pytest.mark.unit
@pytest.mark.xfail(reason="will be fixed by #2199")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

#2222 (review).

It's the same problem as the above link, except this PR's approach is to break the code by releasing with a package that does not pass correctness tests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We discussed this in the dev meeting, and the decision was to add this flag temporarily. Small numerical differences in different versions of dependencies can already be causing problems that we haven't seen. For example, this workflow fails due to jax-finufft=1.2.0, currently this workflow is the only one that uses that version because test_compute_everything happens to run on the runner that uses Python=3.10. What I mean is we don't test each dependency Python version combination on CI, and if certain parts of the code are too sensitive to these, there's not much to do.

There won't be a release very soon. In the meantime, we can merge the proper fix #2199. I can approve that PR and we can merge it soon, but it would be a lot easier if you spent a couple of minutes to cherry-pick a couple of commits to point it to master. Otherwise, it will wait on a chain of PRs. I can say the same for couple other PRs too. You can keep the original PRs open, I am pretty sure if you merge the exact same changes to master, there won't be merge conflicts.

@unalmis unalmis Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I worked on DESC for 4 years. I will block PRs that 1) show failure in the correctness of the code, and 2) remove required testing infrastructure as means to silence such failures.

The other things you mentioned are non-sequitur.

Nothing is holding up the PRs you mentioned. My existing PRs that allow the code to work in a broader range of configurations have been ready for merge with no issues.

@unalmis unalmis Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the meantime, we can merge the proper fix #2199.

You need #2170 and #2199 ; reasons why have been discussed in prs. The ordering of the PR is deliberate for correctness.

def test_z1_first(self):
"""Case where straight line through first two intersects is in epigraph."""
start = np.pi / 3
Expand Down
Loading