Skip to content

Commit 368e28e

Browse files
authored
fix(optim,losses): audited correctness fixes (Adan, SM3, losses) (#838)
* remove unnecessary files * fix(optim,losses): resolve 5 audited C/H/M issues; add regression tests - Adan.update crashed every call (lax.cond operand splat) and froze its step counter, disabling bias correction + the Nesterov term (2 Critical) - SM3 raised KeyError on scalar (0-dim) variables (High) - multi_margin_loss crashed on bm.Array under JAX>=0.9 (High) - l1_loss functional default reduction 'sum' -> 'mean' to match L1Loss (Medium) Findings recorded in docs/issues-found-20260619-optim-losses.md
1 parent 7471264 commit 368e28e

8 files changed

Lines changed: 397 additions & 420 deletions

brainpy/losses/comparison.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -570,7 +570,7 @@ def update(self, input: ArrayType, target: ArrayType) -> ArrayType:
570570
return l1_loss(input, target, reduction=self.reduction)
571571

572572

573-
def l1_loss(logits, targets, reduction='sum'):
573+
def l1_loss(logits, targets, reduction='mean'):
574574
r"""Creates a criterion that measures the mean absolute error (MAE) between each element in
575575
the logits :math:`x` and targets :math:`y`. It is useful in regression problems.
576576
@@ -1045,6 +1045,10 @@ def multi_margin_loss(predicts, targets, margin=1.0, p=1, reduction='mean'):
10451045
a scalar representing the multi-class margin loss. If `reduction` is ``'none'``, then :math:`(N)`.
10461046
"""
10471047
assert p == 1 or p == 2, 'p should be 1 or 2'
1048+
# Convert to plain JAX arrays: under JAX >= 0.9 implicit __jax_array__
1049+
# coercion was removed, so advanced-indexing a ``bm.Array`` would raise.
1050+
predicts = bm.as_jax(predicts)
1051+
targets = bm.as_jax(targets)
10481052
batch_size = predicts.shape[0]
10491053
correct_scores = predicts[jnp.arange(batch_size), targets]
10501054
margins = jnp.power(jnp.maximum(0, predicts - correct_scores[:, jnp.newaxis] + margin), p)

brainpy/losses/comparison_coverage_test.py

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -221,21 +221,26 @@ def test_class_wrapper(self):
221221
# ---------------------------------------------------------------------------
222222
class TestRegressionLosses:
223223
def test_l1_loss_reductions(self):
224-
# NOTE: l1_loss now delegates to braintools.metric.l1_loss, which
225-
# computes the per-row MEAN absolute error (not the L1 norm) for
226-
# reduction='none', then sums / means those per-row values.
224+
# P1-L1: l1_loss delegates to braintools.metric.l1_loss, which for
225+
# reduction='none' returns the per-row L1 *norm* (sum of abs over the
226+
# trailing axes, reshaped to (N, -1)), NOT the per-row mean. So for the
227+
# (2, 2) input below the 'none' output is the per-row sums [3, 7]; 'sum'
228+
# then totals them (10) and 'mean' averages them (5). (The previous
229+
# expectations of [1.5, 3.5]/5/2.5 encoded an incorrect per-row-mean
230+
# assumption about braintools and were pre-existing baseline failures.)
227231
x = jnp.array([[1., 2.], [3., 4.]])
228232
y = jnp.zeros((2, 2))
229233
none = np.asarray(C.l1_loss(x, y, reduction='none'))
230-
assert np.allclose(none, [1.5, 3.5]) # per-row mean abs error
231-
assert float(C.l1_loss(x, y, reduction='sum')) == pytest.approx(5.0)
232-
assert float(C.l1_loss(x, y, reduction='mean')) == pytest.approx(2.5)
234+
assert np.allclose(none, [3.0, 7.0]) # per-row L1 norm (sum of abs)
235+
assert float(C.l1_loss(x, y, reduction='sum')) == pytest.approx(10.0)
236+
assert float(C.l1_loss(x, y, reduction='mean')) == pytest.approx(5.0)
233237

234238
def test_l1_class(self):
235239
x = jnp.array([[1., 2.], [3., 4.]])
236240
y = jnp.zeros((2, 2))
237241
layer = C.L1Loss(reduction='sum')
238-
assert float(layer.update(x, y)) == pytest.approx(5.0)
242+
# sum over per-row L1 norms [3, 7] = 10.0
243+
assert float(layer.update(x, y)) == pytest.approx(10.0)
239244

240245
def test_l2_loss_elementwise(self):
241246
out = np.asarray(C.l2_loss(jnp.array([2.0, 0.0]), jnp.array([0.0, 0.0])))

brainpy/losses/comparison_test.py

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,16 @@ def test_mean_absolute_error(self):
9292
class TestReductionDefaults(unittest.TestCase):
9393
"""Public default ``reduction`` values must not change."""
9494

95-
def test_l1_loss_default_is_sum(self):
95+
def test_l1_loss_default_is_mean(self):
96+
# P1-M1 fix: the functional ``l1_loss`` default reduction is now
97+
# ``'mean'`` (matching the ``L1Loss`` class, the docstring and PyTorch),
98+
# not the surprising ``'sum'`` it previously defaulted to.
9699
pred, tar = _arr(3, 4, seed=1), _arr(3, 4, seed=2)
97100
self.assertTrue(_close(L.l1_loss(pred, tar),
98-
L.l1_loss(pred, tar, reduction='sum')))
101+
L.l1_loss(pred, tar, reduction='mean')))
102+
# and it must equal the OO wrapper's default.
103+
self.assertTrue(_close(L.l1_loss(pred, tar),
104+
L.L1Loss().update(pred, tar)))
99105

100106
def test_mse_default_is_mean(self):
101107
pred, tar = _arr(3, 4, seed=1), _arr(3, 4, seed=2)
@@ -186,5 +192,30 @@ def test_multi_margin_loss_runs(self):
186192
self.assertIsNotNone(L.multi_margin_loss(logits, targets))
187193

188194

195+
class TestMultiMarginArrayEnvelope(unittest.TestCase):
196+
"""P1-H2: ``multi_margin_loss`` must accept ``bm.Array`` inputs.
197+
198+
Under JAX >= 0.9 implicit ``__jax_array__`` coercion was removed, so
199+
indexing a ``bm.Array`` with ``jnp`` advanced indexing raised
200+
``ValueError: Triggering __jax_array__() ... no longer supported``. Every
201+
other loss in the module accepts ``bm.Array``; this one must too.
202+
"""
203+
204+
def test_multi_margin_accepts_bm_array(self):
205+
predicts = bm.asarray(np.array([[0.2, 0.8], [0.6, 0.4]]))
206+
targets = bm.asarray(np.array([1, 0]))
207+
out = L.multi_margin_loss(predicts, targets, margin=1.0, p=1, reduction='mean')
208+
self.assertTrue(np.isfinite(float(out)))
209+
210+
def test_multi_margin_bm_matches_jax(self):
211+
p_np = np.array([[0.2, 0.8, 0.1], [0.6, 0.4, 0.9]])
212+
t_np = np.array([1, 0])
213+
bm_out = np.asarray(L.multi_margin_loss(bm.asarray(p_np), bm.asarray(t_np),
214+
p=2, reduction='none'))
215+
jax_out = np.asarray(L.multi_margin_loss(bm.as_jax(bm.asarray(p_np)),
216+
t_np, p=2, reduction='none'))
217+
self.assertTrue(_close(bm_out, jax_out))
218+
219+
189220
if __name__ == '__main__':
190221
unittest.main()

brainpy/optim/optimizer.py

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
from typing import Union, Sequence, Dict, Optional, Tuple
1818

1919
import jax.numpy as jnp
20-
from jax.lax import cond
2120

2221
import brainpy.math as bm
2322
from brainpy import check
@@ -772,6 +771,10 @@ def __init__(
772771
self.eps = eps
773772
self.weight_decay = weight_decay
774773
self.no_prox = no_prox
774+
# Per-update step counter for bias correction (see ``Adam``). It must be
775+
# independent of the LR scheduler's ``last_epoch`` (which the optimizer
776+
# never advances) and advance exactly once per ``update()``.
777+
self.step = bm.Variable(jnp.asarray(0))
775778

776779
def __repr__(self):
777780
return (f"{self.__class__.__name__}(lr={self.lr}, "
@@ -804,18 +807,26 @@ def _update_moments(self, m, n, v, pre_g, g):
804807

805808
def update(self, grads: dict):
806809
self.check_grads(grads)
807-
lr = self.lr()
808-
step = self.lr.last_epoch.value + 1
809-
correct_m = 1 / (1 - (1 - self.betas[0]) ** (step + 1))
810-
correct_v = 1 / (1 - (1 - self.betas[1]) ** (step + 1))
811-
correct_n = 1 / (1 - (1 - self.betas[2]) ** (step + 1))
810+
lr = bm.as_jax(self.lr())
811+
# Advance the per-update step counter (t = 1 on the first update). The
812+
# bias-correction terms use ``(1 - beta) ** t`` (matching the reference
813+
# Adan), so they actually evolve over training instead of being frozen.
814+
self.step.value = self.step.value + 1
815+
step = self.step.value
816+
correct_m = 1 / (1 - (1 - self.betas[0]) ** step)
817+
correct_v = 1 / (1 - (1 - self.betas[1]) ** step)
818+
correct_n = 1 / (1 - (1 - self.betas[2]) ** step)
812819
for key, p_var in self.vars_to_train.items():
813820
m_var = self.implicit_vars[key + '_m']
814821
n_var = self.implicit_vars[key + '_n']
815822
v_var = self.implicit_vars[key + '_v']
816823
prev_g_var = self.implicit_vars[key + '_prev_grad']
817824
g = grads[key]
818-
pre_g = cond(step == 0, lambda pg, g: g, lambda pg, g: pg, (prev_g_var.value, g))
825+
# On the first update there is no previous gradient, so the gradient
826+
# difference must be 0 (i.e. ``pre_g := g``). Use a value-level
827+
# ``where`` rather than ``lax.cond`` (whose operand is splatted into
828+
# the branch functions, which was the source of the crash).
829+
pre_g = jnp.where(step == 1, g, prev_g_var.value)
819830
diff = g - pre_g
820831
m = m_var.value * (1 - self.betas[0]) + self.betas[0] * g
821832
v = v_var.value * (1 - self.betas[1]) + self.betas[1] * diff
@@ -1082,6 +1093,13 @@ def register_train_vars(self, train_vars: Optional[Dict[str, bm.Variable]] = Non
10821093
vs = dict()
10831094
for k, v in train_vars.items():
10841095
rank, ndim = v.shape, v.ndim
1096+
if ndim == 0:
1097+
# A 0-dim (scalar) variable has no axes to build a cover over.
1098+
# Register a single scalar accumulator so SM3 degenerates to an
1099+
# Adagrad-like update (otherwise ``update`` would ``KeyError`` on
1100+
# the missing ``{k}_m0`` accumulator).
1101+
vs[f'{k}_m0'] = bm.Variable(bm.zeros((), dtype=v.dtype))
1102+
continue
10851103
for i in range(ndim):
10861104
shape = [1] * ndim
10871105
shape[i] = rank[i]
@@ -1098,7 +1116,9 @@ def update(self, grads: dict):
10981116

10991117
for k, p in self.vars_to_train.items():
11001118
g = grads[k]
1101-
ndim = p.ndim
1119+
# Match the rank-1 fallback used when registering accumulators for
1120+
# scalar variables (see ``register_train_vars``).
1121+
ndim = max(p.ndim, 1)
11021122
update = self.implicit_vars[f'{k}_m0']
11031123
for i in range(1, ndim):
11041124
update = bm.minimum(update, self.implicit_vars[f'{k}_m{i}'])

brainpy/optim/optimizer_coverage_test.py

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -185,17 +185,16 @@ def test_zero_norm_branch(self):
185185

186186

187187
class TestAdan:
188-
def test_update_is_currently_broken(self):
189-
# NOTE (defect): Adan.update calls
190-
# cond(step == 0, lambda pg, g: g, lambda pg, g: pg, (prev_g_var.value, g))
191-
# jax.lax.cond unpacks *operands, so passing the single 2-tuple binds
192-
# pg=(prev, g) and leaves g unbound -> TypeError. Adan.update therefore
193-
# cannot run as written (both no_prox branches are unreachable).
188+
def test_update_runs(self):
189+
# P1-C1/P1-C2 fix: Adan.update used to crash (jax.lax.cond operand
190+
# splatting) and its step counter was frozen at 0. It now runs and the
191+
# per-update step counter advances.
194192
v = _make_var(2.0)
195193
opt = O.Adan(lr=1e-2, train_vars={'w': v})
196194
assert 'no_prox' in repr(opt)
197-
with pytest.raises(TypeError):
198-
opt.update({'w': bm.as_jax(v.value)})
195+
out = _train(opt, {'w': v}, lambda val: val)
196+
assert np.isfinite(out['w'])
197+
assert int(bm.as_jax(opt.step.value)) == 5
199198

200199
def test_invalid_eps(self):
201200
with pytest.raises(ValueError):
@@ -245,16 +244,15 @@ def test_invalid_hyperparams(self):
245244

246245

247246
class TestSM3:
248-
def test_scalar_var_is_broken(self):
249-
# NOTE (defect): SM3.register_train_vars loops ``for i in range(ndim)``;
250-
# for a scalar (0-dim) variable no ``_m{i}`` accumulator gets created,
251-
# yet ``update`` reads ``{k}_m0`` -> KeyError. SM3 only works for
252-
# >=1-dim variables.
247+
def test_scalar_var_runs(self):
248+
# P1-H1 fix: SM3 used to KeyError('w_m0') for a scalar (0-dim) variable
249+
# because no accumulator was registered. It now registers a single
250+
# scalar accumulator (Adagrad-like) and updates correctly.
253251
v = _make_var(2.0)
254252
opt = O.SM3(lr=0.1, train_vars={'w': v})
255253
assert 'beta' in repr(opt)
256-
with pytest.raises(KeyError):
257-
opt.update({'w': bm.as_jax(v.value)})
254+
out = _train(opt, {'w': v}, lambda val: np.ones_like(val))
255+
assert np.isfinite(out['w'])
258256

259257
def test_1d_var(self):
260258
v = bm.Variable(bm.asarray(np.array([1.0, 2.0], dtype=np.float32)))

brainpy/optim/optimizer_test.py

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
# Copyright 2025 BrainX Ecosystem Limited. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
# ==============================================================================
15+
"""Regression tests for the 2026-06-19 optim/losses audit.
16+
17+
These pin the bug fixes for:
18+
19+
- P1-C1/P1-C2: ``Adan.update`` crashed on every call (``jax.lax.cond`` operand
20+
mis-binding) and its step counter was frozen at 0 (bias correction / Nesterov
21+
term disabled).
22+
- P1-H1: ``SM3`` raised ``KeyError`` for scalar (0-dim) trainable variables.
23+
"""
24+
25+
import numpy as np
26+
27+
import brainpy.math as bm
28+
from brainpy.optim import optimizer as O
29+
30+
31+
def _vec_var(values):
32+
return bm.Variable(bm.asarray(np.asarray(values, dtype=np.float32)))
33+
34+
35+
def _scalar_var(value=2.0):
36+
return bm.Variable(bm.asarray(np.asarray(value, dtype=np.float32)))
37+
38+
39+
def _train(opt, var_dict, grad_fn, steps=5):
40+
for _ in range(steps):
41+
grads = {k: grad_fn(bm.as_jax(v.value)) for k, v in var_dict.items()}
42+
opt.update(grads)
43+
return {k: np.asarray(bm.as_jax(v.value)) for k, v in var_dict.items()}
44+
45+
46+
# ---------------------------------------------------------------------------
47+
# Adan (P1-C1, P1-C2)
48+
# ---------------------------------------------------------------------------
49+
class TestAdanFixed:
50+
def test_adan_runs_and_updates(self):
51+
# Previously raised TypeError on the very first update.
52+
v = _vec_var([2.0, -3.0])
53+
opt = O.Adan(lr=1e-2, train_vars={'w': v})
54+
out = _train(opt, {'w': v}, lambda val: val, steps=5)
55+
assert np.all(np.isfinite(out['w']))
56+
# gradient = value -> the parameter should move toward zero.
57+
assert np.all(np.abs(out['w']) < np.array([2.0, 3.0]))
58+
59+
def test_adan_no_prox_runs(self):
60+
v = _vec_var([2.0, -3.0])
61+
opt = O.Adan(lr=1e-2, train_vars={'w': v}, no_prox=True)
62+
out = _train(opt, {'w': v}, lambda val: val, steps=5)
63+
assert np.all(np.isfinite(out['w']))
64+
65+
def test_adan_step_counter_advances(self):
66+
v = _vec_var([1.0])
67+
opt = O.Adan(lr=1e-2, train_vars={'w': v})
68+
for _ in range(4):
69+
opt.update({'w': bm.as_jax(v.value)})
70+
assert int(bm.as_jax(opt.step.value)) == 4
71+
72+
def test_adan_first_step_diff_is_zero(self):
73+
# On the very first update the gradient difference (g - g_prev) must be
74+
# treated as 0, so the exp_avg_diff (``_v``) accumulator stays 0 after
75+
# one step regardless of the gradient magnitude.
76+
v = _vec_var([5.0])
77+
opt = O.Adan(lr=1e-2, train_vars={'w': v})
78+
opt.update({'w': bm.as_jax(v.value)})
79+
v_diff = np.asarray(bm.as_jax(opt.implicit_vars['w_v'].value))
80+
assert np.allclose(v_diff, 0.0)
81+
82+
def test_adan_nesterov_term_active_after_two_steps(self):
83+
# With the step counter frozen the diff term was permanently 0; here a
84+
# changing gradient must produce a non-zero exp_avg_diff after step 2.
85+
v = _vec_var([1.0])
86+
opt = O.Adan(lr=1e-2, train_vars={'w': v})
87+
opt.update({'w': np.asarray([1.0], dtype=np.float32)})
88+
opt.update({'w': np.asarray([3.0], dtype=np.float32)}) # gradient changed
89+
v_diff = np.asarray(bm.as_jax(opt.implicit_vars['w_v'].value))
90+
assert not np.allclose(v_diff, 0.0)
91+
92+
93+
# ---------------------------------------------------------------------------
94+
# SM3 (P1-H1)
95+
# ---------------------------------------------------------------------------
96+
class TestSM3Fixed:
97+
def test_sm3_scalar_var_runs(self):
98+
# Previously raised KeyError('w_m0') for a 0-dim variable.
99+
v = _scalar_var(2.0)
100+
opt = O.SM3(lr=0.1, train_vars={'w': v})
101+
out = _train(opt, {'w': v}, lambda val: np.ones_like(val), steps=4)
102+
assert np.all(np.isfinite(out['w']))
103+
# gradient is +1 each step -> scalar parameter must decrease.
104+
assert float(out['w']) < 2.0
105+
106+
def test_sm3_scalar_matches_adagrad_like_step(self):
107+
# For a scalar with constant gradient g=1, SM3 reduces to an Adagrad-like
108+
# update: cache accumulates g^2, step = lr * g / sqrt(cache + eps).
109+
v = _scalar_var(0.0)
110+
opt = O.SM3(lr=0.1, train_vars={'w': v}, eps=1e-30)
111+
opt.update({'w': np.asarray(1.0, dtype=np.float32)})
112+
# after one step cache = 1, update = 0.1 * 1 / sqrt(1) = 0.1
113+
assert float(bm.as_jax(v.value)) == np.float32(-0.1)
114+
115+
def test_sm3_scalar_still_works_with_momentum(self):
116+
v = _scalar_var(2.0)
117+
opt = O.SM3(lr=0.1, train_vars={'w': v}, momentum=0.5, beta=0.5)
118+
out = _train(opt, {'w': v}, lambda val: np.ones_like(val), steps=3)
119+
assert np.all(np.isfinite(out['w']))

0 commit comments

Comments
 (0)