Skip to content

Fix MSTDPET.reset_state_variables to clear p_plus, p_minus, and moving-average buffer - #777

Closed
saachigoyall wants to merge 2 commits into
BindsNET:masterfrom
saachigoyall:master
Closed

Fix MSTDPET.reset_state_variables to clear p_plus, p_minus, and moving-average buffer#777
saachigoyall wants to merge 2 commits into
BindsNET:masterfrom
saachigoyall:master

Conversation

@saachigoyall

Copy link
Copy Markdown
Contributor

Summary

MSTDPET.reset_state_variables (in bindsnet/learning/MCC_learning.py) only zeroed eligibility and eligibility_trace. It left p_plus, p_minus, and the moving-average buffer (average_buffer / average_buffer_index) untouched, so calling network.reset_state_variables() did not fully reset an MSTDPET connection's learning state.

Motivation

In episodic RL training, this caused state to silently persist across episode boundaries even when the network was explicitly reset between episodes, contaminating the start of each new episode with leftover state from the previous one.

Changes

  • MSTDPET.reset_state_variables now also zeros p_plus, p_minus, and (when average_update > 0) average_buffer, and resets
    average_buffer_index to 0.
  • Added test_mstdpet_reset_clears_moving_average_buffer to test/network/test_learning.py, which builds an MSTDPET connection on a MulticompartmentConnection with average_update enabled, runs it long enough to populate all of the above state, calls reset_state_variables(), and asserts everything is zeroed.

@Hananel-Hazan

Copy link
Copy Markdown
Collaborator

Hi Saachi,

Thanks, the diagnosis is right. MSTDPET was clearing 2 of its 6 variables and your one line fix is correct.

Three things came out of checking it:

  1. The fix has no observable effect on its own. network.reset_state_variables() never reaches the learning rule. AbstractFeature.reset_state_variables is the only place that forwards to self.learning_rule, and every concrete feature (Weight, Probability, Mask, MeanField, Bias, Intensity, Degradation) overrides it with a bare pass and never calls super(), so that line is unreachable. I ran your case on a real network: after the reset nothing is zeroed, not even eligibility and eligibility_trace that the current code does clear. The episode contamination you saw comes from this, not from the incomplete MSTDPET reset.

  2. The test does not run. tc_plus, tc_minus, average_update and continues_update go to MulticompartmentConnection, not to Weight. Weight.__init__ has a fixed signature with no **kwargs and never accepted them, so it raises TypeError: Weight.__init__() got an unexpected keyword argument tc_plus. It fails on master and on your own branch, so I think it was never run.

  3. The branch is 40 commits behind master, from June 24. Master now has a fast path in MSTDP that keeps the previous step spikes, and that has to be cleared on reset too, otherwise the first step of an episode pairs with the last step of the previous one. That is the same bug you were chasing.

I wanted to push the wider fix onto your branch so this PR stays yours, but my token does not have the workflow scope and bringing master in touches .github/workflows, so GitHub refused the push. Instead it is in #794, with you as co-author on the commit. It covers all seven features plus MSTDP and PostPre, which cleared nothing at all, and replaces the test with eight cases, seven of which fail on master.

Closing this one in favour of #794. The bug is yours, thanks for finding it. Please do send more.

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.

2 participants