|
| 1 | +# P9 — dyn/synapses + dyn/projections audit (2026-06-19) |
| 2 | + |
| 3 | +Branch: `fix/audit-20260619-dyn-synapses` |
| 4 | +Scope: `brainpy/dyn/synapses/{abstract_models,bio_models,delay_couplings}.py`, |
| 5 | +`brainpy/dyn/projections/{align_post,align_pre,base,conn,delta,inputs,plasticity,utils,vanilla}.py` |
| 6 | +(+ co-located `*_test.py`). |
| 7 | + |
| 8 | +Note on prior audit (`dev/issues-found-20260618.md`): several previously-reported |
| 9 | +synapse/projection bugs are **already fixed in this tree** and were re-verified as |
| 10 | +not-present: C-06/H-39 (STP facilitation `u` uses decayed locals — see the |
| 11 | +documented comment + `u = u + pre_spike*U*(1-u); x = x - pre_spike*u*x`), C-17 |
| 12 | +(PoissonInput uses `scale=sqrt(b*p)` std, not variance), C-18 |
| 13 | +(`HalfProjAlignPost.update` reuses `current` instead of calling `comm` twice), |
| 14 | +C-19/H-41 (`STDP_Song2000` passes `W_min/W_max=None` through unchanged and uses |
| 15 | +`bm.as_jax` on traces), H-40 (`projections/base.py` now re-exports the real |
| 16 | +`Projection`/`SynConn`). These are noted for the record only. |
| 17 | + |
| 18 | +--- |
| 19 | + |
| 20 | +### P9-H1 — DualExpon raises ZeroDivisionError / yields NaN when `tau_rise == tau_decay` [High] |
| 21 | +- File: brainpy/dyn/synapses/abstract_models.py:159-164,265-266 |
| 22 | +- Category: numerics / edge / api-drift |
| 23 | +- What: `_format_dual_exp_A` computes the peak normalizer |
| 24 | + `A = tau_decay/(tau_decay - tau_rise) * (tau_rise/tau_decay)**(tau_rise/(tau_rise-tau_decay))`. |
| 25 | + When `tau_rise == tau_decay` the leading factor divides by zero. For Python-float |
| 26 | + taus this raises `ZeroDivisionError` at construction; for array taus it silently |
| 27 | + produces `inf`, and `DualExpon`'s `a = (tau_decay-tau_rise)/(tau_rise*tau_decay)*A` |
| 28 | + then becomes `0*inf = nan`. |
| 29 | +- Why it's a bug: equal rise/decay time constants is a legitimate, common request |
| 30 | + (the dual-exponential degenerates to the normalized alpha function). It is also the |
| 31 | + parameterization used by the `dynold` `AlphaCUBA/AlphaCOBA` compat classes (C-20), |
| 32 | + which route through `DualExpon`. A crash / silent NaN on a realistic default is wrong. |
| 33 | +- Repro: `bp.dyn.DualExpon(2, tau_rise=10., tau_decay=10.)` -> `ZeroDivisionError`; |
| 34 | + `bp.dyn.DualExpon(2, tau_rise=bm.asarray([10.,5.]), tau_decay=bm.asarray([10.,50.]))` |
| 35 | + -> `a = [nan, 0.258...]`. |
| 36 | +- Fix: compute `DualExpon.a` via the closed form `a = (1/tau_rise) * |
| 37 | + (tau_rise/tau_decay)**(tau_rise/(tau_rise-tau_decay))` (algebraically equal to the |
| 38 | + old expression but free of the `(tau_decay-tau_rise)` cancellation), and special-case |
| 39 | + the equal-tau limit element-wise to its L'Hôpital value `a = e/tau` (verified: a |
| 40 | + single-spike response then peaks at exactly 1.0). The `A is None` auto-normalizer path |
| 41 | + only; an explicitly supplied `A` is honoured unchanged. |
| 42 | +- Tests: `abstract_models_test.py::TestDualExpon::test_equal_tau_no_crash`, |
| 43 | + `::test_equal_tau_matches_alpha` |
| 44 | +- Status: fixed |
| 45 | + |
| 46 | +### P9-H2 — DualExponV2 silently outputs all-zeros (or NaN) when `tau_rise == tau_decay` [High] |
| 47 | +- File: brainpy/dyn/synapses/abstract_models.py:159-164,413 |
| 48 | +- Category: numerics / edge |
| 49 | +- What: `DualExponV2` stores `self.a = A` and returns `a * (g_decay - g_rise)`. With |
| 50 | + equal taus the two gates obey identical ODEs with identical inputs, so |
| 51 | + `g_decay - g_rise ≡ 0` for all time; the synapse is structurally singular. With the |
| 52 | + default `A=None` normalizer this additionally hits the same div-by-zero/`inf` as P9-H1, |
| 53 | + giving `inf*0 = nan`. |
| 54 | +- Why it's a bug: the model produces a silently-dead (identically zero) or NaN synapse |
| 55 | + for an innocuous parameter choice, with no diagnostic. Unlike `DualExpon`, no finite |
| 56 | + `A` can recover a non-zero waveform, so the only correct behaviour is a clear error. |
| 57 | +- Repro: `bp.dyn.DualExponV2(2, tau_rise=10., tau_decay=10.)` -> `ZeroDivisionError`; |
| 58 | + with an explicit `A=1.` the output is identically 0 over a full simulation. |
| 59 | +- Fix: in `_format_dual_exp_A`, when `A is None` and `tau_rise == tau_decay`, raise a |
| 60 | + clear `ValueError` telling the user the dual-exponential normalizer is undefined for |
| 61 | + equal time constants and to use `brainpy.dyn.Alpha` (single-tau alpha synapse) instead. |
| 62 | + (Only the V2/auto-normalizer path reaches this branch; `DualExpon` computes `a` |
| 63 | + directly per P9-H1 and never calls the helper for the equal-tau case.) |
| 64 | +- Tests: `abstract_models_test.py::TestDualExpon::test_v2_equal_tau_raises` |
| 65 | +- Status: fixed |
| 66 | + |
| 67 | +### P9-H3 — STP.reset_state crashes for per-neuron (array) `U` [High] |
| 68 | +- File: brainpy/dyn/synapses/abstract_models.py:855-858 |
| 69 | +- Category: correctness / edge |
| 70 | +- What: `reset_state` does `self.u = self.init_variable(bm.ones, ...)` then |
| 71 | + `self.u.fill_(self.U)`. `fill_` requires a scalar fill value (`shape == ()`), so when |
| 72 | + `U` is a per-neuron array the call raises |
| 73 | + `MathError: The shape of the fill value must be ()`. |
| 74 | +- Why it's a bug: heterogeneous release probability `U` across synapses is a standard, |
| 75 | + realistic short-term-plasticity configuration; the model crashes on construction. |
| 76 | +- Repro: `bp.dyn.STP(3, U=bm.asarray([0.1, 0.2, 0.3]))` -> `MathError`. |
| 77 | +- Fix: initialise `u` by broadcasting `U` into the (possibly batched) state: |
| 78 | + `self.u = self.init_variable(bm.ones, batch_or_mode); self.u.value = self.u.value * self.U`. |
| 79 | + Works for scalar `U`, array `U`, and batched modes. |
| 80 | +- Tests: `abstract_models_test.py::TestSTP::test_array_U_reset`, |
| 81 | + `::test_array_U_run` |
| 82 | +- Status: fixed |
| 83 | + |
| 84 | +### P9-M1 — STP/STD discrete jumps assume binary `pre_spike`; graded inputs scaled wrongly [Medium] |
| 85 | +- File: brainpy/dyn/synapses/abstract_models.py:800-801,883-884 |
| 86 | +- Category: numerics |
| 87 | +- What: the "simplified" updates `x = x - pre_spike*U*self.x` (STD) and |
| 88 | + `u = u + pre_spike*U*(1-u); x = x - pre_spike*u*x` (STP) reproduce the original |
| 89 | + `bm.where(pre_spike, ...)` exactly only for `pre_spike in {0,1}`. For graded |
| 90 | + (non-binary) presynaptic signals the depression/facilitation magnitude is scaled by |
| 91 | + the graded value, which is not the documented Tsodyks–Markram jump. |
| 92 | +- Why it's a bug: callers feeding graded "spikes" get a different (linearly scaled) |
| 93 | + release, with no warning. (Matches prior-audit M-22.) |
| 94 | +- Repro: static — compare `bm.where(pre>0, x-U*x, x)` vs `x-pre*U*x` for `pre=0.5`. |
| 95 | +- Fix: recorded only. The simplified form is a defensible generalization, is faithful to |
| 96 | + the historical `dynold` STD/STP convention for the binary case, and changing it risks |
| 97 | + altering long-standing numeric behaviour. Documenting/asserting binary input is a |
| 98 | + cross-cutting API decision left to maintainers. |
| 99 | +- Tests: none |
| 100 | +- Status: recorded-only |
| 101 | + |
| 102 | +### P9-M2 — STD applies depression jump to the pre-decay state, inconsistent with the STP fix [Medium] |
| 103 | +- File: brainpy/dyn/synapses/abstract_models.py:795-801 |
| 104 | +- Category: numerics |
| 105 | +- What: `STD.update` integrates `x -> x_decayed` but then applies the release jump using |
| 106 | + the **pre-decay** Variable: `self.x.value = x_decayed - pre_spike*U*self.x`. The |
| 107 | + companion `STP` model was deliberately changed (prior-audit H-39, documented comment) |
| 108 | + to apply jumps to the **decayed** local `x`. STD was left on the pre-decay form, so the |
| 109 | + two short-term-plasticity models now discretize the spike jump inconsistently (off by |
| 110 | + one decay step in the jump term). |
| 111 | +- Why it's a bug: an asymmetry/correctness smell; the decayed-local form is the cleaner |
| 112 | + discretization. |
| 113 | +- Repro: static. |
| 114 | +- Fix: recorded only. The pre-decay form matches the original commented code AND the |
| 115 | + `dynold` reference (`short_term_plasticity.py` STD), and the per-step difference is |
| 116 | + `O(dt/tau)` confined to the jump term. Changing it alters historical STD numerics for |
| 117 | + every user; flagged for a maintainer decision rather than fixed unilaterally. |
| 118 | +- Tests: none |
| 119 | +- Status: recorded-only |
| 120 | + |
| 121 | +### P9-L1 — DelayCoupling docstrings reference a non-existent gain `g`; malformed `Parameters::` [Low] |
| 122 | +- File: brainpy/dyn/synapses/delay_couplings.py:131-163,234-256 |
| 123 | +- Category: style |
| 124 | +- What: `DiffusiveCoupling`/`AdditiveCoupling` docstrings describe |
| 125 | + `coupling = g * (...)` but no `g` gain parameter exists (the gain is folded into |
| 126 | + `conn_mat`). They also use the literal-block `Parameters::` marker instead of a |
| 127 | + NumPy-doc underlined `Parameters` section (CLAUDE.md mandates NumPy-doc). Matches |
| 128 | + prior-audit L-09/L-14. |
| 129 | +- Why it's a bug: misleading docs / Sphinx rendering. |
| 130 | +- Fix: recorded only (Low). |
| 131 | +- Tests: none |
| 132 | +- Status: recorded-only |
| 133 | + |
| 134 | +### P9-L2 — GABAa docstring lists wrong default alpha/beta [Low] |
| 135 | +- File: brainpy/dyn/synapses/bio_models.py:286-287 |
| 136 | +- Category: style |
| 137 | +- What: the `Args` block says `alpha: Default 0.062` and `beta: Default 3.57`, but the |
| 138 | + constructor defaults are `alpha=0.53`, `beta=0.18`. |
| 139 | +- Why it's a bug: documentation does not match code. |
| 140 | +- Fix: recorded only (Low). |
| 141 | +- Tests: none |
| 142 | +- Status: recorded-only |
| 143 | + |
| 144 | +### P9-L3 — SynConn.update raises with non-f-string message; conn_mat shape check on raw input [Low] |
| 145 | +- File: brainpy/dyn/projections/conn.py:119; delay_couplings.py:81 |
| 146 | +- Category: style / edge |
| 147 | +- What: minor robustness/style items (e.g. `AdditiveCoupling.update`'s final |
| 148 | + `else: raise ValueError` has no message; `conn_mat.shape` is read before confirming |
| 149 | + the object exposes `.shape`). |
| 150 | +- Fix: recorded only (Low). |
| 151 | +- Tests: none |
| 152 | +- Status: recorded-only |
0 commit comments