-
Notifications
You must be signed in to change notification settings - Fork 370
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
Conversation
Before (current master): After (code in this PR): |
3a5a5fa
to
adb0a5b
Compare
models/stdp_synapse.h
Outdated
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does:
nest-simulator/models/clopath_synapse.h
Line 215 in 720f4c5
double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/ht_synapse.h
Line 189 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/jonke_synapse.h
Line 300 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/quantal_stp_synapse.h
Line 195 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/stdp_dopamine_synapse.h
Line 529 in 720f4c5
double t_spike = e.get_stamp().get_ms(); |
double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/stdp_nn_restr_synapse.h
Line 229 in 720f4c5
double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/stdp_nn_symm_synapse.h
Line 231 in 720f4c5
double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/stdp_pl_synapse_hom.h
Line 244 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/stdp_synapse_hom.h
Line 281 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/stdp_triplet_synapse.h
Line 243 in 720f4c5
double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/tsodyks2_synapse.h
Line 209 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/tsodyks_synapse_hom.h
Line 256 in 720f4c5
const double t_spike = e.get_stamp().get_ms(); |
nest-simulator/models/urbanczik_synapse.h
Line 207 in 720f4c5
double t_spike = e.get_stamp().get_ms(); |
double t_spike = e.get_stamp().get_ms(); |
There was a problem hiding this comment.
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.
eaf2454
to
6c11fe7
Compare
There was a problem hiding this 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 ;).
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 + "]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
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.
@clinssen Ping :) |
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. |
There was a problem hiding this 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.
There was a problem hiding this 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: |
There was a problem hiding this comment.
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 + "]" |
There was a problem hiding this comment.
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.
@heplesser, can you please have another look? Thanks! |
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:
Neuron models:
iaf_psc_exp_psTodo:
Addresses (fixes?) #1604.