fix(dynold): STP construction, sparse-synapse drift, plasticity decay, Bellec init#848
Merged
Merged
Conversation
…, Bellec adaptation init - STP.reset_state passed (batch_or_mode, sizes) to variable_ in the wrong order, making STP unconstructable (ValueError); swapped to (sizes, batch) like STD (Critical) - sparse Exponential+stp / DualExponential called csrmv(..., method='cusparse'); csrmv no longer accepts method -> TypeError on every sparse update; removed kwarg (High) - Exponential sparse+stp branch fed raw pre_spike to csrmv instead of the STP-filtered syn_value, silently ignoring STP on sparse layout (High) - STD/STP discrete jumps read pre-decay state instead of the decayed locals (off-by-one decay); use decayed x^-, u^- and facilitated u^+ (Medium) - ALIFBellec2020 / LIF_SFA_Bellec2020 default a_initializer started SFA adaptation deeply negative; changed OneInit(-50.) -> ZeroInit() (Medium) Findings recorded in docs/issues-found-20260619-dynold.md
Reviewer's GuideFixes multiple issues in legacy dynold short‑term plasticity, sparse synapse computation, and Bellec SFA neuron initialization, and adds regression tests plus an audit doc for the dynold package. Sequence diagram for sparse Exponential synapse update with STPsequenceDiagram
participant ExponentialSynapse as Exponential
participant STP as STP_module
participant bm_sparse_csrmv as bm_sparse_csrmv
ExponentialSynapse->>STP: __call__(pre_spike)
STP-->>ExponentialSynapse: stp_factor
ExponentialSynapse->>ExponentialSynapse: syn_value = stp_factor * pre_spike
ExponentialSynapse->>bm_sparse_csrmv: csrmv(data, indices, indptr, syn_value, shape, transpose=True)
bm_sparse_csrmv-->>ExponentialSynapse: post_vs
ExponentialSynapse-->>ExponentialSynapse: update returns post_vs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The STD/STP jump logic is now duplicated between
experimental.syn_plasticityandsynplast.short_term_plasticity; consider factoring the Tsodyks–Markram update into a shared helper or base to avoid future drift between the two implementations. - Several new STP/STD tests encode the exact numerical evolution (e.g., hand-computing
x_dec,u_decwith a specificdtand integrator expectation); you may want to loosen these slightly or centralize the expected update formula so they remain robust if the default ODE method or time-step handling changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The STD/STP jump logic is now duplicated between `experimental.syn_plasticity` and `synplast.short_term_plasticity`; consider factoring the Tsodyks–Markram update into a shared helper or base to avoid future drift between the two implementations.
- Several new STP/STD tests encode the exact numerical evolution (e.g., hand-computing `x_dec`, `u_dec` with a specific `dt` and integrator expectation); you may want to loosen these slightly or centralize the expected update formula so they remain robust if the default ODE method or time-step handling changes.
## Individual Comments
### Comment 1
<location path="brainpy/dynold/experimental/syn_plasticity_test.py" line_range="96-105" />
<code_context>
+ np.testing.assert_allclose(bm.as_jax(stp.x.value), np.ones(4))
+ np.testing.assert_allclose(bm.as_jax(stp.u.value), np.full(4, 0.15))
+
+ def test_stp_update_state_changes(self):
+ # P11-C1 regression: a constructed STP must update without error and
+ # respond to a presynaptic spike (u facilitates, x depresses).
+ stp = sp.STP(4, U=0.15, tau_f=1500., tau_d=200.)
+ share.save(t=0.0, dt=bm.dt)
+ x_before = bm.as_jax(stp.x.value).copy()
+ u_before = bm.as_jax(stp.u.value).copy()
+ r = bm.as_jax(stp.update(bm.ones(4, dtype=bool)))
+ self.assertEqual(r.shape, (4,))
+ self.assertTrue(np.all(bm.as_jax(stp.u.value) >= u_before - 1e-6))
+ self.assertTrue(np.all(bm.as_jax(stp.x.value) <= x_before + 1e-6))
def _make_stp(self, num=4, U=0.15, tau_f=1500., tau_d=200.):
</code_context>
<issue_to_address>
**issue (testing):** The regression test for STP update only checks weak inequalities and could pass even if the spike has no effect.
In `test_stp_update_state_changes`, the checks `u >= u_before - 1e-6` and `x <= x_before + 1e-6` would still pass if `u` and `x` never change, so the test doesn’t actually guarantee that spikes modify the state. To make this a true regression, consider either (1) asserting that at least one element satisfies `u > u_before + eps` and `x < x_before - eps` for a small `eps`, or (2) checking against the analytically expected Tsodyks–Markram jump given `U`, `tau_f`, and `tau_d`, so the test will fail if STP stops responding to presynaptic spikes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+96
to
+105
| def test_stp_update_state_changes(self): | ||
| # P11-C1 regression: a constructed STP must update without error and | ||
| # respond to a presynaptic spike (u facilitates, x depresses). | ||
| stp = sp.STP(4, U=0.15, tau_f=1500., tau_d=200.) | ||
| share.save(t=0.0, dt=bm.dt) | ||
| x_before = bm.as_jax(stp.x.value).copy() | ||
| u_before = bm.as_jax(stp.u.value).copy() | ||
| r = bm.as_jax(stp.update(bm.ones(4, dtype=bool))) | ||
| self.assertEqual(r.shape, (4,)) | ||
| self.assertTrue(np.all(bm.as_jax(stp.u.value) >= u_before - 1e-6)) |
There was a problem hiding this comment.
issue (testing): The regression test for STP update only checks weak inequalities and could pass even if the spike has no effect.
In test_stp_update_state_changes, the checks u >= u_before - 1e-6 and x <= x_before + 1e-6 would still pass if u and x never change, so the test doesn’t actually guarantee that spikes modify the state. To make this a true regression, consider either (1) asserting that at least one element satisfies u > u_before + eps and x < x_before - eps for a small eps, or (2) checking against the analytically expected Tsodyks–Markram jump given U, tau_f, and tau_d, so the test will fail if STP stops responding to presynaptic spikes.
github-actions
Bot
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fresh review of legacy
brainpy/dynold.STP.reset_statepassed args tovariable_in the wrong order, making STP unconstructable.Exponential+stp/DualExponentialpassedmethod='cusparse'tocsrmvwhich no longer accepts it ->TypeErroron every sparse update.Exponential+stpfed rawpre_spiketocsrmvinstead of the STP-filteredsyn_value, silently ignoring STP on sparse layout.x^-, u^-and facilitatedu^+(Tsodyks–Markram).ALIFBellec2020/LIF_SFA_Bellec2020default adaptation initOneInit(-50.)->ZeroInit().In-scope: 554 passed. Findings:
docs/issues-found-20260619-dynold.md.Summary by Sourcery
Fix multiple regressions in legacy dynold short-term plasticity, sparse synapses, and Bellec neuron defaults, and add regression tests and documentation for the audited issues.
Bug Fixes:
Enhancements:
Documentation:
Tests: