Fix STDP k-value error for edge case

[JanVogelsang]

This pull requests aims to fix issue #2437 .
Regression test "issue-2437.py" provides a minimal example of the error in NEST which fails for the current master.
The issue is fixed by changing the condition to remove spikes from the post-synaptic neuron's spike history in the set_spiketime function.

The problem to be fixed in this PR happens due to communication on a fixed grid (i.e. in fixed intervals) instead of instantly. As STDP weight updates are only performed right upon sending/communicating a spike, the weight update will not use the state of the history of the post-synaptic neuron when the spike occurs, but when the spike is communicated instead. This can lead to scenarios where the history is updated between two communication episodes and spikes that are still relevant for the weight update might get removed. To prevent early removal of spikes, the time window in the condition of the set_spiketime function for which spikes are kept in the history is extended by the min_delay (time between two communication cycles).

[JanVogelsang]

Apart from the added regression test, there is only a single line changes in this PR, which adds the (global) min_delay in the ArchivingNode::set_spiketime to make sure spikes are not removed from the history too early.

Unfortunetaly, introducing this change makes one test fail: Pipeline step:9:4671
Does someone have an idea what causes the "numerical instability"?

[clinssen]

Thanks for finding this! I Why is a new test necessary? Could you add the offending pre/post spike sequence from the linked issue to one of the existing STDP synapse tests?

[JanVogelsang]

Alright, I merged the existing stdp_synapse test and the regression test I created for the issue to now cover everything in a single test. I also fixed some bugs in the existing test that I stumbled across when writing the regression test, so the test will now be much safer (e.g. when comparing floating point numbers).

[JanVogelsang]

Fails due to numerical instability, without any obvious reason (at least not obvious to me).

[heplesser]

Fails due to numerical instability, without any obvious reason (at least not obvious to me).

The aeif models are notoriously problematic with respect to numerics. You could try to stabilise this by changing in

nest-simulator/testsuite/regressiontests/issue-77.sli

Lines 107 to 110 in e899520

	/aeif_cond_beta_multisynapse << /params << /E_rev [ 0.0 ]
	/tau_rise [ 1.0 ]
	/tau_decay [ 2.0 ] >>
	/receptor_type 1 >>

the value for E_rev to something lower, e.g., /E_rev [ -20.0 ].

See also #2420.

[clinssen]

Fails due to numerical instability, without any obvious reason (at least not obvious to me).

The aeif models are notoriously problematic with respect to numerics. You could try to stabilise this by changing in

nest-simulator/testsuite/regressiontests/issue-77.sli

Lines 107 to 110 in e899520

	/aeif_cond_beta_multisynapse << /params << /E_rev [ 0.0 ]
	/tau_rise [ 1.0 ]
	/tau_decay [ 2.0 ] >>
	/receptor_type 1 >>

the value for E_rev to something lower, e.g., /E_rev [ -20.0 ].

See also #2420.

It is not clear why this should pop up in this PR, as the aeif models are not touched by it.

@JanVogelsang: could you merge upstream/master to check this not just a fluke?

[heplesser]

Fails due to numerical instability, without any obvious reason (at least not obvious to me).

The aeif models are notoriously problematic with respect to numerics. You could try to stabilise this by changing in

It is not clear why this should pop up in this PR, as the aeif models are not touched by it.

@JanVogelsang: could you merge upstream/master to check this not just a fluke?

issue-77.sli tests that all neuron models correctly support STDP by driving them with a strong Poisson input. Presumably, the changes to the STDP code pushed aeif_cond_beta_multisynapse across the edge of instability. As a fix, you can also add /V_peak -30.0 to the /params dictionary shown above. You could do the same for the other aeif neurons in the test.

[JanVogelsang]

Unfortunately, setting V_peak alone does not fix the instability yet. Setting E_rev to -20.0 does fix it though. Should I therefore just set E_rev (and if so, in which models, as only the aeif_cond_alpha_multisynapse seems to fail)?

[heplesser]

Unfortunately, setting V_peak alone does not fix the instability yet. Setting E_rev to -20.0 does fix it though. Should I therefore just set E_rev (and if so, in which models, as only the aeif_cond_alpha_multisynapse seems to fail)?

Yes, then change E_rev in that test as well. I would only change it for the model where the test fails.

[clinssen]

This is just a small request: could you undo the changes that reformat to an 80 character line length? Lines of code in NEST Simulator are now wrapped at 120 characters.

[JanVogelsang]

@clinssen Could you check again if this is ready to be merged now?

[clinssen]

Great, thank you!