Make network.reset_state_variables() actually reach the learning rules - #794
Merged
Merged
Conversation
… rules Builds on @saachigoyall's fix. Her diagnosis was right: MSTDPET cleared only 2 of its 6 state variables. But that change alone had no observable effect, because the reset never reached any learning rule in the first place. AbstractFeature.reset_state_variables forwards to self.learning_rule, and it is the only place that does. Every concrete feature overrode it with a bare 'pass' and none called super(), so the forwarding line was unreachable. After network.reset_state_variables() nothing was cleared, not even the two variables MSTDPET already handled. Features (topology_features.py): - Drop the seven bare-'pass' overrides (Probability, Mask, MeanField, Weight, Bias, Intensity, Degradation) so they inherit the base implementation, and drop @AbstractMethod from it, which is why those overrides existed at all. - The two adaptation features keep their own reset logic and now chain to super() first. Rules (MCC_learning.py): - MSTDPET now also clears p_plus, p_minus and the moving-average buffer (@saachigoyall's change). - MSTDP cleared nothing. It now clears eligibility, p_plus, p_minus, the moving-average buffer, and the fast path's one-step spike lag. That lag is newer than this PR: without clearing it, the first step of an episode pairs with the last step of the previous one, which is the contamination this PR set out to fix. - PostPre cleared nothing; it now clears both averaging buffers. - Hebbian holds no state; its no-op is now documented as deliberate. - MSTDP's lazily-built state (_prev_source_s, _prev_target_s, eligibility) is declared None in __init__ and guarded with 'is None' rather than hasattr, so reset has something defined to restore. Tests: replaces the original test, which passed tc_plus/average_update to Weight (whose signature never accepted them) and so raised TypeError rather than running. Eight cases now, seven of which fail without the source change, including an end-to-end check that two identical episodes separated by a reset produce identical weights. Full suite 91 passed. Per-step update cost unchanged over three interleaved A/B rounds at n=64 and n=256, all differences under 1% with the sign varying between rounds. Co-Authored-By: Saachi Goyal <156711741+saachigoyall@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hananel-Hazan
added a commit
that referenced
this pull request
Sep 7, 2026
Brings in the six commits master gained today: four dependency bumps, the removal of the dead AbstractFeature.degrade hook (#793), the learning-rule reset fix (#794), and its follow-up for MSTDP's lazily built state (#795). No textual conflicts. The two files both sides touched, MCC_learning.py and topology_features.py, merged cleanly and the result is correct in both directions: this branch's perf work (the MSTDP fast path, the fold cache, the cached decay tensors) is intact, and master's reset fix reaches the learning rules through the feature chain as intended. Full suite on the merge: 182 passed, which includes this branch's test_perf_equivalence.py and test_learning_rule_specs.py alongside master's new reset tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hananel-Hazan
added a commit
that referenced
this pull request
Sep 7, 2026
DiehlAndCook arrived on this branch while the reset audit (#794) was happening on master, so it kept the bare 'return' that the audit removed everywhere else. It holds no state between steps: each update comes from the source trace, the target spikes and the current weight, exactly like Hebbian. So the no-op is correct, but it now says so rather than looking like the oversight the other rules turned out to be. Extends the two reset tests to cover Hebbian and DiehlAndCook as well. Full suite 186 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hananel-Hazan
added a commit
that referenced
this pull request
Sep 7, 2026
Follow-up to #794. MSTDP creates p_plus and p_minus lazily on the first update, because only then are the batch size and device known. #794 made reset_state_variables zero them unconditionally, so calling network.reset_state_variables() before the first run raised AttributeError: 'MSTDP' object has no attribute 'p_plus'. Building a network and resetting it before the first episode is a normal thing to do, so this was reachable. Declare p_plus and p_minus as None in __init__ alongside the other lazily built state, switch the update path's hasattr guards to 'is None' to match, and have the reset skip whatever has not been built. MSTDPET was never affected: it builds both in __init__. New test parametrised over MSTDP, MSTDPET and PostPre resets a freshly built network before running it. It fails for MSTDP without this change. Full suite 94 passed. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Hananel-Hazan
added a commit
that referenced
this pull request
Sep 16, 2026
* docs: add SECURITY.md with reporting process and incident note * perf: fuse STDP updates, drop per-step identity matrices, cache MSTDP constants, in-place neuron state; fix network.to() on MulticompartmentConnection - PostPre/Hebbian (classic + MCC) apply the outer-product update with one addmm_ - LearningRule.update skips the w *= 1.0 multiply when no weight decay is set - Local-connection rules scale rows instead of bmm with torch.eye per step - MSTDP/MSTDPET cache exp(-dt/tc) and default a_plus/a_minus tensors - Nodes update v/refrac_count/theta/x in place (same ops, same order) - rank_order encoding vectorised - Fix AbstractMulticompartmentConnection._apply(recurse) so network.to(device) works on DiehlAndCook2015 - test/network/test_perf_equivalence.py pins each change to its reference formula - examples/benchmark/hot_path_bench.py; CLAUDE.md + .vscode point at the bindsNET env Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * validate learning rules against their source papers - test/network/test_learning_rule_specs.py: PostPre / WeightDependentPostPre / Hebbian vs Morrison, Diesmann & Gerstner (2008) eqs 11-14; Rmax vs Vasilaki et al. (2009) eqs 7, 8, 13; MCC PostPre/Hebbian vs classic; Diehl & Cook deviation pinned - test_mstdp_florian.py: Florian (2007) equation numbers recorded - Fix reversed Rmax tc_c docstring and misleading MSTDP/MSTDPET zero_lag comments - docs/source/models_spec.rst: cite the equations per rule - CHANGELOG entry Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix: clamp/unclamp spikes now enter the spike traces; document rule sources - Nodes.forward applies Network.run's clamp/unclamp before the trace update (previously applied after, so forced spikes never reached the traces the learning rules read; affected supervised_mnist.py) - Tests: TestClampEntersTraces; STDP window test now also driven by clamp - README section 'Learning rules and their sources'; models_spec.rst clamp and known-deviations sections; CLAUDE.md rule/validation notes; CHANGELOG Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * learning: add Diehl & Cook (2015) rule, drop dt factor from MCC PostPre, move rule docs - bindsnet.learning.DiehlAndCook + MCC_learning.DiehlAndCook: post-spike-only dw = eta (x_pre - x_tar)(w_max - w)^mu (Diehl & Cook 2015 Sect. 2.3); DiehlAndCook2015(learning_rule=..., learning_rule_kwargs=...) opt-in, default PostPre unchanged - MCC_learning.PostPre no longer scales updates by dt (per-spike increment, as the classic rule and Morrison 2008 eqs 13-14) - Weight feature forwards extra kwargs to its learning rule - Tests: DiehlAndCook vs paper reference (mu, x_tar, both trace modes), MCC vs classic at dt 1.0 and 0.5, dt-independence of MCC PostPre, model opt-in - Docs: bindsnet/learning/README.md (moved from top-level README), models_spec, CLAUDE.md, CHANGELOG Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix: MSTDP reset must tolerate state that is not built yet Follow-up to #794. MSTDP creates p_plus and p_minus lazily on the first update, because only then are the batch size and device known. #794 made reset_state_variables zero them unconditionally, so calling network.reset_state_variables() before the first run raised AttributeError: 'MSTDP' object has no attribute 'p_plus'. Building a network and resetting it before the first episode is a normal thing to do, so this was reachable. Declare p_plus and p_minus as None in __init__ alongside the other lazily built state, switch the update path's hasattr guards to 'is None' to match, and have the reset skip whatever has not been built. MSTDPET was never affected: it builds both in __init__. New test parametrised over MSTDP, MSTDPET and PostPre resets a freshly built network before running it. It fails for MSTDP without this change. Full suite 94 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * learning: give DiehlAndCook the same documented reset as Hebbian DiehlAndCook arrived on this branch while the reset audit (#794) was happening on master, so it kept the bare 'return' that the audit removed everywhere else. It holds no state between steps: each update comes from the source trace, the target spikes and the current weight, exactly like Hebbian. So the no-op is correct, but it now says so rather than looking like the oversight the other rules turned out to be. Extends the two reset tests to cover Hebbian and DiehlAndCook as well. Full suite 186 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: restrict GITHUB_TOKEN to contents:read in all workflows CodeQL alerts 10, 11 and 12 (actions/missing-workflow-permissions) flag that none of the three workflows declare a permissions block, so each run inherits the repository default token scope. None of the three write back to the repository: black.yml runs psf/black, python-app.yml runs flake8 and pytest, pythonpackage.yml runs black and pytest across Python 3.11-3.13. Read-only access to the repository contents is sufficient for all of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * security: document pickle risk in load(); fix three broken torch.load sites bindsnet.network.load() reads a Python pickle file via torch.load, so loading a network file from an untrusted source runs code in that file. This is standard torch.load behaviour across the PyTorch ecosystem rather than a defect specific to BindsNET, and it is not being treated as a vulnerability, but it was completely undocumented. Adds a warning to each affected function's docstring and a "Loading saved networks and models" section to SECURITY.md. load() gains a weights_only parameter passed through to torch.load. It defaults to False, so behaviour is unchanged: Network.save() stores the whole network object rather than a tensor state dict, and the safe loader cannot read those files at all. Reversing the default would break every saved network. Reviewing the neighbouring call sites turned up three things that were simply broken, all from PyTorch 2.6 flipping the weights_only default to True: - Network.clone() raised UnpicklingError. No test, no caller in the tree. - Network.save() called add_safe_globals([self]) with an instance where a class is expected. It did nothing useful and corrupted PyTorch's safe-globals registry, so any later load in the same process failed with 'Network' object has no attribute '__qualname__'. Removed. - conversion.ann_to_snn and data_based_normalization raised UnpicklingError when given a path instead of a module. Only the in-memory form was tested. SpokenMNIST now reads its processed-data cache with weights_only=True. That cache holds only tensors, so refusing code execution there costs nothing. It is the one place the reporter's suggested fix actually applies. Six regression tests added, each carrying the reason in its docstring. No per-timestep path is touched, so performance is unaffected. Full suite: 193 passed. Reported-by: Gavin Branaa <gbranaa4@gmail.com> Thanks to Gavin for the careful private report, and for the follow-up that led to the three fixes above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * release: publish 0.3.4 to PyPI Add a trusted-publishing workflow (runs on GitHub Release or by hand), drop install requirements nothing imports (Cython, scikit-build, foolbox, numba), loosen torch/torchvision to >=2.14,<3 / >=0.29,<1, point README and install docs at pip install bindsnet, and record in CHANGELOG that PyPI 0.3.4 is master on 2026-09-16, not the 0.3.4 tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci+packaging: fix CI-only test failures, Poetry 2.4.3, [project] metadata, working Dockerfile - test_perf_equivalence: batch-1 fused addmm_ checks accept float32 rounding (bit equality is CPU dependent; 5 tests failed on GitHub runners) and warn with the size of the difference. - Poetry 2.4.3 in CI and docs; metadata moved to [project]; lock resolves to the same packages; poetry-core>=2.0; unused setup.py removed. - Dockerfile rewritten (old base image, installer and .python-version gone). - publish.yml uses download-artifact v8; old repo links fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: record CPU-dependent rounding of the fused addmm_ path Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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
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.
Supersedes #777. Co-authored with @saachigoyall, who found and diagnosed the bug.
What is wrong
AbstractFeature.reset_state_variablesforwards the reset toself.learning_rule, and it is the only place that does. Every concrete feature overrode it with a barepassand none calledsuper(), so that forwarding line was unreachable.The result:
network.reset_state_variables()never reached anyMulticompartmentConnectionlearning rule. Driving an MSTDPET connection for 250 steps and then resetting the network left every state variable untouched, including the two thatMSTDPET.reset_state_variablesalready cleared.On top of that, the rules themselves were incomplete. MSTDPET cleared 2 of its 6 variables (#777). MSTDP and PostPre cleared nothing at all.
Changes
bindsnet/network/topology_features.pypassoverrides (Probability,Mask,MeanField,Weight,Bias,Intensity,Degradation) so they inherit the base implementation.@abstractmethodfromAbstractFeature.reset_state_variables, which is why those overrides existed in the first place. This only relaxes the contract: a subclass may still override.AdaptationBaseSynapsHistoryandAdaptationBaseOtherSynapskeep their own reset logic and now chain tosuper()first.bindsnet/learning/MCC_learning.pyMSTDPETnow also clearsp_plus,p_minusand the moving-average buffer (@saachigoyall's change).MSTDPnow clearseligibility,p_plus,p_minus, the moving-average buffer, and the fast path's one-step spike lag. That lag is newer than Fix MSTDPET.reset_state_variables to clear p_plus, p_minus, and moving-average buffer #777, and it matters: without clearing it the first step of an episode pairs with the last step of the previous one, which is exactly the cross-episode contamination Fix MSTDPET.reset_state_variables to clear p_plus, p_minus, and moving-average buffer #777 set out to fix.PostPrenow clears both averaging buffers and their indices.Hebbianholds no state between steps; its no-op is now documented as deliberate rather than looking like an oversight.MSTDP's lazily-built state (_prev_source_s,_prev_target_s,eligibility) is declaredNonein__init__and guarded withis Noneinstead ofhasattr, so reset has something well-defined to restore.Tests
Eight new cases in
TestLearningRuleReset. Seven of the eight fail against master, so they genuinely pin the bug. The strongest one runs two identical episodes with a reset between them and requires identical resulting weights.This replaces the test from #777, which passed
tc_plusandaverage_updatetoWeight.Weight.__init__has a fixed signature that never accepted them, so that test raisedTypeErrorrather than running, on master and on its own branch alike.Full suite: 91 passed (83 before).
black --checkandisortclean on the changed files.Performance
Reset runs once per episode, not per time step, so the added clearing is off the hot path. Two edits do touch the per-step update path (
hasattrtois None), so I measured: three interleaved A/B rounds against master, MSTDP fast path, MSTDPET, and MSTDP dense path, at n=64 and n=256. All differences under 1% with the sign varying between rounds, i.e. no measurable change.🤖 Generated with Claude Code