Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add STDP synapse unit testing #1840

Merged
merged 24 commits into from
Oct 27, 2021
Merged

Conversation

clinssen
Copy link
Contributor

@clinssen clinssen commented Nov 12, 2020

This adds a unit test for the STDP synapse, by comparing the weight evolution due to a barrage of pre- and postsynaptic spikes between NEST and a Python reference implementation.

Test stimuli:

  • randomly generated (Poisson distribution) spike trains
  • hard-coded edge cases such as simultaneous pre and post spikes

Neuron models:

  • iaf_psc_exp
  • iaf_cond_exp
  • iaf_psc_exp_ps

Todo:

  • Unit test code cleanup
  • Add documentation

Addresses (fixes?) #1604.

@clinssen clinssen marked this pull request as draft November 12, 2020 16:46
@clinssen clinssen added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Nov 23, 2020
@clinssen
Copy link
Contributor Author

clinssen commented May 3, 2021

test_stdp_multiplicity has been fixed. The condition for test failure was not appropriate and rewritten. Performance is much better now. The following figure shows how, for smaller and smaller timestep, the final obtained weights of a "normal" versus a "precise" single STDP synapse converge.

Before (current master):

image

After (code in this PR):

image

@clinssen clinssen force-pushed the test-stdp-synapse branch 4 times, most recently from 3a5a5fa to adb0a5b Compare May 3, 2021 19:54
@@ -217,7 +217,7 @@ inline void
stdp_synapse< targetidentifierT >::send( Event& e, thread t, const CommonSynapseProperties& )
{
// synapse STDP depressing/facilitation dynamics
const double t_spike = e.get_stamp().get_ms();
const double t_spike = e.get_stamp().get_ms() - e.get_offset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clinssen This change makes me wonder if we have the same error in other synapse models as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does:

double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

const double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

double t_spike = e.get_stamp().get_ms();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clinssen So this means that until now, we have not taken spike time offsets into account in plasticity mechanisms, doesn't it? It does not have any effect on the actual timing of the spike transmitted, since the offset information is passed on to the receiving neuron in the Event object. Could you create a follow-up issue for this? We should involve @abigailm in the discussion.

@clinssen clinssen force-pushed the test-stdp-synapse branch 7 times, most recently from eaf2454 to 6c11fe7 Compare May 4, 2021 13:33
@clinssen clinssen marked this pull request as ready for review May 4, 2021 14:37
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clinssen This looks quite good, but some of the code should be simplified or better documented; tests need to be comprehensible ;).

pynest/nest/tests/test_stdp_multiplicity.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_stdp_multiplicity.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_stdp_synapse.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_stdp_synapse.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_stdp_synapse.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_stdp_synapse.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_stdp_synapse.py Outdated Show resolved Hide resolved
for self.nest_neuron_model in ["iaf_psc_exp",
"iaf_cond_exp",
"iaf_psc_exp_ps"]:
fname_snip = "_[nest_neuron_mdl=" + self.nest_neuron_model + "]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A snippet inserted into the filename when saving debug plots to disk.

@heplesser
Copy link
Contributor

@clinssen Ping :)

@clinssen
Copy link
Contributor Author

I removed all changes related to support for precise spike timing in synapses, and test coverage for precise neurons. This feature discussion will be continued in #2035.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clinssen Just a few more comments.

testsuite/pytests/test_stdp_multiplicity.py Outdated Show resolved Hide resolved
testsuite/pytests/test_stdp_multiplicity.py Outdated Show resolved Hide resolved
testsuite/pytests/test_stdp_synapse.py Outdated Show resolved Hide resolved
testsuite/pytests/test_stdp_nn_synapses.py Show resolved Hide resolved
Copy link
Contributor

@YounesBouhadjar YounesBouhadjar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me now, I just left a small comment

idx_next_post_spike = np.where((post_spikes_delayed - t) > 0)[0][0]
t_next_post_spike = post_spikes_delayed[idx_next_post_spike]

if idx_next_post_spike >= 0 and t_next_post_spike < t_next_pre_spike:
Copy link
Contributor

@YounesBouhadjar YounesBouhadjar Oct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t_next_post_spike or t_next_pre_spike might be referenced before assignment, if pre_spikes or post_spikes_delayed are empty. Maybe it's ok here, as we know that this never occurs in this specific test case.

for self.dendritic_delay in [1., self.resolution]:
self.init_params()
for self.nest_neuron_model in ["iaf_psc_exp", "iaf_cond_exp"]:
fname_snip = "_[nest_neuron_mdl=" + self.nest_neuron_model + "]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you include the dendritic delay in the name here? otherwise, the corresponding figures of self.resolution will overwrite the figures of dendritic delay = 1.

@jougs
Copy link
Contributor

jougs commented Oct 21, 2021

@heplesser, can you please have another look? Thanks!

@heplesser heplesser merged commit cfa80ce into nest:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants