You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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:
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.
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.
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.
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
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.
Summary
MSTDPET.reset_state_variables(inbindsnet/learning/MCC_learning.py) only zeroedeligibilityandeligibility_trace. It leftp_plus,p_minus, and the moving-average buffer (average_buffer/average_buffer_index) untouched, so callingnetwork.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_variablesnow also zerosp_plus,p_minus, and (whenaverage_update > 0)average_buffer, and resetsaverage_buffer_indexto 0.test_mstdpet_reset_clears_moving_average_buffertotest/network/test_learning.py, which builds an MSTDPET connection on aMulticompartmentConnectionwithaverage_updateenabled, runs it long enough to populate all of the above state, callsreset_state_variables(), and asserts everything is zeroed.