Skip to content

Make network.reset_state_variables() actually reach the learning rules - #794

Merged
Hananel-Hazan merged 1 commit into
masterfrom
fix/learning-rule-reset
Sep 7, 2026
Merged

Hananel-Hazan merged 1 commit into
masterfrom
fix/learning-rule-reset

Conversation

@Hananel-Hazan

Copy link
Copy Markdown
Collaborator

Supersedes #777. Co-authored with @saachigoyall, who found and diagnosed the bug.

What is wrong

AbstractFeature.reset_state_variables forwards the reset 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 that forwarding line was unreachable.

The result: network.reset_state_variables() never reached any MulticompartmentConnection learning rule. Driving an MSTDPET connection for 250 steps and then resetting the network left every state variable untouched, including the two that MSTDPET.reset_state_variables already 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.py

  • Drop the seven bare-pass overrides (Probability, Mask, MeanField, Weight, Bias, Intensity, Degradation) so they inherit the base implementation.
  • Drop @abstractmethod from AbstractFeature.reset_state_variables, which is why those overrides existed in the first place. This only relaxes the contract: a subclass may still override.
  • AdaptationBaseSynapsHistory and AdaptationBaseOtherSynaps keep their own reset logic and now chain to super() first.

bindsnet/learning/MCC_learning.py

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_plus and average_update to Weight. Weight.__init__ has a fixed signature that never accepted them, so that test raised TypeError rather than running, on master and on its own branch alike.

Full suite: 91 passed (83 before). black --check and isort clean 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 (hasattr to is 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

… 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
Hananel-Hazan merged commit f12870f into master Sep 7, 2026
10 checks passed
@Hananel-Hazan
Hananel-Hazan deleted the fix/learning-rule-reset branch September 7, 2026 02:10
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant