Add symmetric stdp synapse to NEST #218

Merged
merged 29 commits into from Jul 5, 2016

Conversation

Projects
None yet
8 participants
@sanjayankur31
Contributor

sanjayankur31 commented Feb 1, 2016

Reference:
Vogels et al. (2011) Inhibitory Plasticity Balances Excitation and
Inhibition in Sensory Pathways and Memory Networks. Science Vol. 334,
Issue 6062, pp. 1569-1573 DOI: 10.1126/science.1211095

http://www.sciencemag.org/content/334/6062/1569.abstract
Also adds a python test file to the manualtests folders.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 1, 2016

Contributor

The test python file in manualtests generates this graph:
https://github.com/sanjayankur31/nest-symmetric-stdp-test/blob/master/results/gnuplot.png

Things I'm not sure about:

  • I'm not so sure of the case where spikes are co-incident. Even when I didn't have the conditional, I didn't get any facilitation. I'm wondering if this has something with the way nest does things?
  • Do I need to implement the hpc variant too? And a hom variant?
Contributor

sanjayankur31 commented Feb 1, 2016

The test python file in manualtests generates this graph:
https://github.com/sanjayankur31/nest-symmetric-stdp-test/blob/master/results/gnuplot.png

Things I'm not sure about:

  • I'm not so sure of the case where spikes are co-incident. Even when I didn't have the conditional, I didn't get any facilitation. I'm wondering if this has something with the way nest does things?
  • Do I need to implement the hpc variant too? And a hom variant?
models/stdp_symmetric_connection.h
@@ -0,0 +1,300 @@
+/*
+ * stdp_connection.h

This comment has been minimized.

@tammoippen

tammoippen Feb 2, 2016

Contributor

In the copyright notice, the correct file name should be stated: stdp_symmetric_connection.h. This is why regressiontests/ticket-659-copyright.py fails.

@tammoippen

tammoippen Feb 2, 2016

Contributor

In the copyright notice, the correct file name should be stated: stdp_symmetric_connection.h. This is why regressiontests/ticket-659-copyright.py fails.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Feb 2, 2016

Contributor

Also the formatting of some of the file seems to be off, e.g. see TravisCI output. Please make sure, that you use clang-format version 3.6 for the formatting.

Contributor

tammoippen commented Feb 2, 2016

Also the formatting of some of the file seems to be off, e.g. see TravisCI output. Please make sure, that you use clang-format version 3.6 for the formatting.

sanjayankur31 added some commits Jan 13, 2016

Add symmetric stdp synapse to NEST
Reference:
Vogels et al. (2011) Inhibitory Plasticity Balances Excitation and
Inhibition in Sensory Pathways and Memory Networks.  Science Vol. 334,
Issue 6062, pp. 1569-1573 DOI: 10.1126/science.1211095

http://www.sciencemag.org/content/334/6062/1569.abstract

Also adds a python test file to the manualtests folders.
Format files using clang-format 3.6.0
The version in Fedora is 3.7.x which was causing the tests to fail.
@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 2, 2016

Contributor

@tammoippen - Thank you for notes on the test failures. The new set of commits seem to be OK.

Contributor

sanjayankur31 commented Feb 2, 2016

@tammoippen - Thank you for notes on the test failures. The new set of commits seem to be OK.

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Feb 15, 2016

Contributor

@flinz @suku248 would be good second reviewers. @sanjayankur31 : a non-manual test would be a great addition.

Contributor

jakobj commented Feb 15, 2016

@flinz @suku248 would be good second reviewers. @sanjayankur31 : a non-manual test would be a great addition.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

Thanks @jakobj - I'll look into writing a non-manual test.

Contributor

sanjayankur31 commented Feb 15, 2016

Thanks @jakobj - I'll look into writing a non-manual test.

models/stdp_symmetric_connection.h
+ facilitate_( double_t w, double_t kplus )
+ {
+ double_t norm_w = ( w / Wmax_ ) + ( lambda_ * eta_ * kplus );
+ return norm_w < 1.0 ? norm_w * Wmax_ : Wmax_;

This comment has been minimized.

@flinz

flinz Feb 15, 2016

Contributor

I believe there is a small error, caused by the weight maximization. Let me demonstrate here, but the same holds for depression.

For given values of w and kplus, you are assigning the new w to be (assuming you don't hit the norm_w < 1.0):

w_returned 
= [( w_old / Wmax_ ) + ( lambda_ * eta_ * kplus )] * Wmax_ 
= w_old + lambda_ * eta_ * kplus * Wmax

This makes the weight updates scale additionally with Wmax, which I believe should not be the case. For facilitation this should rather be:

w_returned 
= w_old + lambda_ * eta_ * kplus 

You can change the lines to something akin to this implementation.

Just as a remark: Looking at the corresponding line in stdp_connection for the additive case (mu_plus=0) I think we get the same scaling with Wmax. @abigailm @mhelias am I getting something wrong or is this an intended behavior?

@flinz

flinz Feb 15, 2016

Contributor

I believe there is a small error, caused by the weight maximization. Let me demonstrate here, but the same holds for depression.

For given values of w and kplus, you are assigning the new w to be (assuming you don't hit the norm_w < 1.0):

w_returned 
= [( w_old / Wmax_ ) + ( lambda_ * eta_ * kplus )] * Wmax_ 
= w_old + lambda_ * eta_ * kplus * Wmax

This makes the weight updates scale additionally with Wmax, which I believe should not be the case. For facilitation this should rather be:

w_returned 
= w_old + lambda_ * eta_ * kplus 

You can change the lines to something akin to this implementation.

Just as a remark: Looking at the corresponding line in stdp_connection for the additive case (mu_plus=0) I think we get the same scaling with Wmax. @abigailm @mhelias am I getting something wrong or is this an intended behavior?

This comment has been minimized.

@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

I wasn't sure about this either, but decided to go with the way stdp_connection is implemented (as you observe above).

@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

I wasn't sure about this either, but decided to go with the way stdp_connection is implemented (as you observe above).

models/stdp_symmetric_connection.h
+ {
+ minus_dt = t_lastspike - ( start->t_ + dendritic_delay );
+ ++start;
+ if ( minus_dt == 0 )

This comment has been minimized.

@flinz

flinz Feb 15, 2016

Contributor

Regarding your comment in #218 (comment): be aware that postsynaptic spikes have to occur shortly before the presynaptic spikes (due to dendritic delay) for this condition to be fulfilled.

Effectively, and this is rather by popular convention, exactly coincident spikes (at the synapse) do not cause a weight change, see e.g. equation (2) here.

@flinz

flinz Feb 15, 2016

Contributor

Regarding your comment in #218 (comment): be aware that postsynaptic spikes have to occur shortly before the presynaptic spikes (due to dendritic delay) for this condition to be fulfilled.

Effectively, and this is rather by popular convention, exactly coincident spikes (at the synapse) do not cause a weight change, see e.g. equation (2) here.

This comment has been minimized.

@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

Well, in the classical STDP, as you point out, delta_w is undefined when delta_t is zero since in the neighbourhood of zero, the values are vastly different.

In the case of this symmetric rule, however, this isn't the case - the value of delta_w in the neighbourhood of zero on either side is the same - in this version of stdp, there should be a maximum facilitation when delta_t is zero. (Figure 1C in the paper).

So, is it OK if I remove the (minus_dt == 0) conditional?

@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

Well, in the classical STDP, as you point out, delta_w is undefined when delta_t is zero since in the neighbourhood of zero, the values are vastly different.

In the case of this symmetric rule, however, this isn't the case - the value of delta_w in the neighbourhood of zero on either side is the same - in this version of stdp, there should be a maximum facilitation when delta_t is zero. (Figure 1C in the paper).

So, is it OK if I remove the (minus_dt == 0) conditional?

This comment has been minimized.

@suku248

suku248 Mar 3, 2016

You mentioned in your above comment that removing the conditional statement doesn't show any effect. This is due the implementation of get_history, which returns the postsynaptic spike history in (t_lastspike, t_spike] such that the condition minus_dt == 0 is never met.

@suku248

suku248 Mar 3, 2016

You mentioned in your above comment that removing the conditional statement doesn't show any effect. This is due the implementation of get_history, which returns the postsynaptic spike history in (t_lastspike, t_spike] such that the condition minus_dt == 0 is never met.

models/stdp_symmetric_connection.h
+ continue;
+ // Use value of kplus at the time of the post synaptic spike
+ // therefore the exponential multiplier
+ // for minus_dt = 0, the exponential becomes 1 and facilitation occurs

This comment has been minimized.

@flinz

flinz Feb 15, 2016

Contributor

This code is never reached if minus_dt=1, the comment should be removed.

@flinz

flinz Feb 15, 2016

Contributor

This code is never reached if minus_dt=1, the comment should be removed.

models/stdp_symmetric_connection.h
+ const CommonSynapseProperties& )
+{
+ // synapse STDP depressing/facilitation dynamics
+ // if(t_lastspike >0) {std::cout << "last spike " << t_lastspike << std::endl ;}

This comment has been minimized.

@flinz

flinz Feb 15, 2016

Contributor

Please remove debugging couts.

@flinz

flinz Feb 15, 2016

Contributor

Please remove debugging couts.

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Feb 15, 2016

Contributor

Hey @sanjayankur31, I had a quick look at your code and the paper.

Before you go on diggin into the details below, here is a general point to consider: The Vogels learning rule is indeed not implementable by the current stdp_connection (there is a constant depression term, independent of k_minus), so this class of learning rules is a sensible addition. However, for the sake of keeping NEST more general, and this class more reusable, it could be beneficial to instead make this a STDP connection (using both tau_plus and tau_minus) with optional constant terms for both depression and facilitation (alpha_fac and alpha_dep). For tau_plus=tau_minus and alpha_fac=0 the current model would be contained as a special case.
@heplesser @jougs what's your opinion on this?

Besides, here are comments on the current class:

  1. Naming: I think the class should be named stdp_connection_symmetric in accordance with, e.g. stdp_connection_hom. One could think about naming this stdp_connection_vogels, since it is a little more specialized than being simply symmetric: it has a constant depression term (see also the general remark above).
  2. Redundancy of lambda: I think the parameter lambda might be redundant. You already introduce the parameter eta (the learning rate, as in the paper) and both only occur as lambda * eta. In this case, to adhere to paper notation, remove lambda from the implementation.
  3. Update description: You should mention in the model description that the essential difference to stdp_connection is the constant depression of synapses, independent of the actual spike times. I think this could be helpful for people browsing models, that are not familiar with the Vogels model.
  4. Expose alpha instead of rho_0: The following might be a matter of discussion, since its rather a question of how general we want NEST models to be implemented.
    The analysis in the paper and identification of rho_0 (named kappa in your implementation) with a target firing rate relies on the feedforward setup of inhibition (Fig. S1).
    I would personally prefer to use for the model implementation only the more general parameter alpha (Eq. 4 in the supplement) and then mention that for alpha = 2 * tau * rho_0 one would recover the exact implementation of Vogels' feedforward scenario.
    This has the advantage that the inhibitory offset alphais directly exposed and controllable, since it does not change with both tau and kappa.
  5. Testing: As you already mentioned above, you should include a proper automated test for the synapse. You can simply adapt for example this test for the triplet rule to check for correct weight changes against the actual equations of Vogels Eqs. S4 & S5.
Contributor

flinz commented Feb 15, 2016

Hey @sanjayankur31, I had a quick look at your code and the paper.

Before you go on diggin into the details below, here is a general point to consider: The Vogels learning rule is indeed not implementable by the current stdp_connection (there is a constant depression term, independent of k_minus), so this class of learning rules is a sensible addition. However, for the sake of keeping NEST more general, and this class more reusable, it could be beneficial to instead make this a STDP connection (using both tau_plus and tau_minus) with optional constant terms for both depression and facilitation (alpha_fac and alpha_dep). For tau_plus=tau_minus and alpha_fac=0 the current model would be contained as a special case.
@heplesser @jougs what's your opinion on this?

Besides, here are comments on the current class:

  1. Naming: I think the class should be named stdp_connection_symmetric in accordance with, e.g. stdp_connection_hom. One could think about naming this stdp_connection_vogels, since it is a little more specialized than being simply symmetric: it has a constant depression term (see also the general remark above).
  2. Redundancy of lambda: I think the parameter lambda might be redundant. You already introduce the parameter eta (the learning rate, as in the paper) and both only occur as lambda * eta. In this case, to adhere to paper notation, remove lambda from the implementation.
  3. Update description: You should mention in the model description that the essential difference to stdp_connection is the constant depression of synapses, independent of the actual spike times. I think this could be helpful for people browsing models, that are not familiar with the Vogels model.
  4. Expose alpha instead of rho_0: The following might be a matter of discussion, since its rather a question of how general we want NEST models to be implemented.
    The analysis in the paper and identification of rho_0 (named kappa in your implementation) with a target firing rate relies on the feedforward setup of inhibition (Fig. S1).
    I would personally prefer to use for the model implementation only the more general parameter alpha (Eq. 4 in the supplement) and then mention that for alpha = 2 * tau * rho_0 one would recover the exact implementation of Vogels' feedforward scenario.
    This has the advantage that the inhibitory offset alphais directly exposed and controllable, since it does not change with both tau and kappa.
  5. Testing: As you already mentioned above, you should include a proper automated test for the synapse. You can simply adapt for example this test for the triplet rule to check for correct weight changes against the actual equations of Vogels Eqs. S4 & S5.
@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

Hi @flinz - thank you very much for your comments. I'm travelling over the next week or so and will address them as soon as get back.

Contributor

sanjayankur31 commented Feb 15, 2016

Hi @flinz - thank you very much for your comments. I'm travelling over the next week or so and will address them as soon as get back.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

One question before I make any more changes - is the suggested practice in nest to simply add more commits on top of the current ones - thus maintaining the complete review and code history, or should I squash commits up to make it all tidy (and then maybe force push)?

Contributor

sanjayankur31 commented Feb 15, 2016

One question before I make any more changes - is the suggested practice in nest to simply add more commits on top of the current ones - thus maintaining the complete review and code history, or should I squash commits up to make it all tidy (and then maybe force push)?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Feb 15, 2016

Contributor

@sanjayankur31: we think it's nicer to just add commits to your branch in order to have a nice and transparent history of code review process.

@flinz: many thanks for your nice and solid review and the quick reaction.

@heplesser: could you make sure the copyright transfer is handled properly? Thanks!

Contributor

jougs commented Feb 15, 2016

@sanjayankur31: we think it's nicer to just add commits to your branch in order to have a nice and transparent history of code review process.

@flinz: many thanks for your nice and solid review and the quick reaction.

@heplesser: could you make sure the copyright transfer is handled properly? Thanks!

sanjayankur31 added some commits Feb 15, 2016

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Feb 15, 2016

Contributor

I've fixed some of the minor issues and pushed commits. I'll fix the major issues as soon as I can.

Contributor

sanjayankur31 commented Feb 15, 2016

I've fixed some of the minor issues and pushed commits. I'll fix the major issues as soon as I can.

sanjayankur31 added some commits Feb 16, 2016

Replace kappa with alpha
As requested, I've replaced kappa with alpha, and added a comment in
the documentation explaining the relation between the two.
+
+ # set up the synapse
+ syn_spec_synapse = {'weight': weight_pre, 'Wmax': 100.,
+ 'kappa': 0.1, 'eta': 0.001,

This comment has been minimized.

@weidel-p

weidel-p Mar 2, 2016

Contributor

When I run this test, I get Unused dictionary items: kappa.
Looking into the default parameter reveals that kappa is indeed not a parameter.

@weidel-p

weidel-p Mar 2, 2016

Contributor

When I run this test, I get Unused dictionary items: kappa.
Looking into the default parameter reveals that kappa is indeed not a parameter.

This comment has been minimized.

@sanjayankur31

sanjayankur31 Mar 2, 2016

Contributor

kappa was replaced by alpha after the review. The manual test file hasn't been updated since it'll be replaced by an automated test (I'm working on the latter)

@sanjayankur31

sanjayankur31 Mar 2, 2016

Contributor

kappa was replaced by alpha after the review. The manual test file hasn't been updated since it'll be replaced by an automated test (I'm working on the latter)

@suku248

This comment has been minimized.

Show comment
Hide comment
@suku248

suku248 Mar 3, 2016

Concerning your question about hpc and hom versions of the model:
The registration in the models module using TargetIdentifierIndex automatically creates an hpc version of your synapse model. For a hom version, however, you would have to add another class but this is not a requirement for the merge of this PR.

suku248 commented Mar 3, 2016

Concerning your question about hpc and hom versions of the model:
The registration in the models module using TargetIdentifierIndex automatically creates an hpc version of your synapse model. For a hom version, however, you would have to add another class but this is not a requirement for the merge of this PR.

@suku248

This comment has been minimized.

Show comment
Hide comment
@suku248

suku248 Mar 3, 2016

Regarding @flinz above comments:
I think that the more general implementation suggested by @flinz is a good idea but not a requirement for merging this PR. And sorry: I preferred stdp_symmetric_connection (or stdp_vogels_connection) as in stdp_triplet_connection or stdp_pl_connection.

suku248 commented Mar 3, 2016

Regarding @flinz above comments:
I think that the more general implementation suggested by @flinz is a good idea but not a requirement for merging this PR. And sorry: I preferred stdp_symmetric_connection (or stdp_vogels_connection) as in stdp_triplet_connection or stdp_pl_connection.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Mar 23, 2016

Contributor

Thank you for your comments @suku248 - I've just pushed the automated test and am waiting for the CI to check the build.

  • About the generalisation - The current implementation permits the user to make changes to the depression constant and other parameters while maintaining the symmetry of the learning rule - given that the point of the learning rule is that it's symmetric, permitting users to make it asymmetric sort of changes the synapse completely - what do you think?
  • @flinz, @suku248 - should I change the name to stdp_symmetric_synapse? It's a rather minor change - (I'd like to avoid calling it Vogels since Vogels and Sprekeler contributed equally to the work and in that scheme I'd like to include both their names)
Contributor

sanjayankur31 commented Mar 23, 2016

Thank you for your comments @suku248 - I've just pushed the automated test and am waiting for the CI to check the build.

  • About the generalisation - The current implementation permits the user to make changes to the depression constant and other parameters while maintaining the symmetry of the learning rule - given that the point of the learning rule is that it's symmetric, permitting users to make it asymmetric sort of changes the synapse completely - what do you think?
  • @flinz, @suku248 - should I change the name to stdp_symmetric_synapse? It's a rather minor change - (I'd like to avoid calling it Vogels since Vogels and Sprekeler contributed equally to the work and in that scheme I'd like to include both their names)
@suku248

This comment has been minimized.

Show comment
Hide comment
@suku248

suku248 Apr 4, 2016

Thanks for the automated test @sanjayankur31

We briefly discussed the naming issue in our weekly core developer meeting and decided that for now vogels_sprekeler_synapse would be a good name for the synapse model.

As previously mentioned: I second @flinz suggestion to make the model more general but I don't think this has to happen with this PR.

suku248 commented Apr 4, 2016

Thanks for the automated test @sanjayankur31

We briefly discussed the naming issue in our weekly core developer meeting and decided that for now vogels_sprekeler_synapse would be a good name for the synapse model.

As previously mentioned: I second @flinz suggestion to make the model more general but I don't think this has to happen with this PR.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Apr 4, 2016

Contributor

@suku248 - thanks - I can work on the generalisation bit later if it isn't required for this PR. Should the source file be renamed to stdp_vogels_sprekeler_connection.h or something similar too, or should I leave that alone?

Contributor

sanjayankur31 commented Apr 4, 2016

@suku248 - thanks - I can work on the generalisation bit later if it isn't required for this PR. Should the source file be renamed to stdp_vogels_sprekeler_connection.h or something similar too, or should I leave that alone?

@suku248

This comment has been minimized.

Show comment
Hide comment
@suku248

suku248 Apr 4, 2016

@sanjayankur31 it should be named vogels_sprekeler_connection.h

suku248 commented Apr 4, 2016

@sanjayankur31 it should be named vogels_sprekeler_connection.h

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Apr 4, 2016

Contributor

@suku248 , @flinz - renamed the synapse. Please let me know if there's anything else. I'll begin working on the generalisation later this week.

Contributor

sanjayankur31 commented Apr 4, 2016

@suku248 , @flinz - renamed the synapse. Please let me know if there's anything else. I'll begin working on the generalisation later this week.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Apr 5, 2016

Contributor

I'm waiting on #284 - once that is merged, I'll update this implementation to use a similar system.

Contributor

sanjayankur31 commented Apr 5, 2016

I'm waiting on #284 - once that is merged, I'll update this implementation to use a similar system.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Apr 6, 2016

Contributor

TODO - add source files to models/CMakeLists.txt

Contributor

sanjayankur31 commented Apr 6, 2016

TODO - add source files to models/CMakeLists.txt

sanjayankur31 added some commits Apr 10, 2016

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 10, 2016

Contributor

@sanjayankur31 Would you pull the newest changes from master so that Travis can check this PR properly now that #391 is merged?

Contributor

heplesser commented Jun 10, 2016

@sanjayankur31 Would you pull the newest changes from master so that Travis can check this PR properly now that #391 is merged?

sanjayankur31 added some commits Jun 10, 2016

Update file to mimic triplet synapse source
This ensures that the synapse can be both excitatory and inhibitory.
@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Jun 20, 2016

Contributor

Hiya, I've updated the source to use an implementation similar to the triplet synapse in #284. So this is finally ready for a complete review and merge :)

Contributor

sanjayankur31 commented Jun 20, 2016

Hiya, I've updated the source to use an implementation similar to the triplet synapse in #284. So this is finally ready for a complete review and merge :)

models/modelsmodule.cpp
@@ -112,6 +112,7 @@
#include "stdp_triplet_connection.h"
#include "stdp_dopa_connection.h"
#include "stdp_pl_connection_hom.h"
+#include "vogels_sprekeler_connection.h"

This comment has been minimized.

@heplesser

heplesser Jun 20, 2016

Contributor

Could you put this in alphabetical order?

@heplesser

heplesser Jun 20, 2016

Contributor

Could you put this in alphabetical order?

models/vogels_sprekeler_connection.h
+//#include "dictdatum.h"
+//#include "connector_model.h"
+//#include "common_synapse_properties.h"
+//#include "event.h"

This comment has been minimized.

@heplesser

heplesser Jun 20, 2016

Contributor

Please remove the commented-out lines.

@heplesser

heplesser Jun 20, 2016

Contributor

Please remove the commented-out lines.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 20, 2016

Contributor

@sanjayankur31 The code looks clean except for two little details, see inline comments.

@flinz @suku248 Would you have a final look before we merge?

Contributor

heplesser commented Jun 20, 2016

@sanjayankur31 The code looks clean except for two little details, see inline comments.

@flinz @suku248 Would you have a final look before we merge?

sanjayankur31 added some commits Jun 20, 2016

@suku248

This comment has been minimized.

Show comment
Hide comment
@suku248

suku248 Jul 5, 2016

Seems fine to me 👍

suku248 commented Jul 5, 2016

Seems fine to me 👍

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jul 5, 2016

Contributor

@sanjayankur31 And merging :).

Contributor

heplesser commented Jul 5, 2016

@sanjayankur31 And merging :).

@heplesser heplesser merged commit 62c019b into nest:master Jul 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sanjayankur31 sanjayankur31 deleted the sanjayankur31:symmetric_stdp_synapse branch Jul 5, 2016

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Aug 9, 2016

Contributor

@heplesser sorry I was on holidays. Too late for the party 👍

Contributor

flinz commented Aug 9, 2016

@heplesser sorry I was on holidays. Too late for the party 👍

aserenko added a commit to aserenko/nest-simulator that referenced this pull request Dec 6, 2017

Add STDP synapses with nearest-neighbor spike pairing schemes
Add three types of nearest-neighbour STDP: symmetric (but not to be
confused with PR #218), presynaptic-centered, and restricted
symmetric. These are worth implementing because described in a highly-
cited review [Morrison A., Diesmann M., and Gerstner W. (2008)
Phenomenological models of synaptic plasticity based on spike timing,
Biol. Cybern. 98, 459--478].

In these models a spike is taken into account in the weight change
rule not with all preceding spikes, but only with some of the
nearest, see the three pairing schemes in Fig. 7 in [Morrison et
al., 2008].

The implementation relies on two additional variables - presynaptic
and postsynaptic traces, like in stdp_synapse. However, their update
rules differ from those of the latter. That's why the archiving_node
was modified to add one more state variable, nearest_neighbor_Kminus.
It is not stored in the node, just computed on the fly when
requested. To return nearest_neighbor_Kminus I changed the
archiving_node.get_K_values() function signature, because, as fas as
I can see, this function is not currently used anywhere.

The tests for all three models reside in a single file (but as three
separate tests), and are the following: generate two Poisson spike
sequences as presynaptic and postsynaptic, feed them to NEST to get
the synaptic weight change, then reproduce the weight change
independently outside of NEST and check whether the expected weight
equals the obtained one.

aserenko added a commit to aserenko/nest-simulator that referenced this pull request Dec 6, 2017

Add STDP synapses with nearest-neighbor spike pairing schemes
Add three types of nearest-neighbour STDP: symmetric (but not to be
confused with PR #218), presynaptic-centered, and restricted
symmetric. These are worth implementing because described in a highly-
cited review [Morrison A., Diesmann M., and Gerstner W. (2008)
Phenomenological models of synaptic plasticity based on spike timing,
Biol. Cybern. 98, 459--478].

In these models a spike is taken into account in the weight change
rule not with all preceding spikes, but only with some of the
nearest, see the three pairing schemes in Fig. 7 in [Morrison et
al., 2008].

The implementation relies on two additional variables - presynaptic
and postsynaptic traces, like in stdp_synapse. However, their update
rules differ from those of the latter. That's why the archiving_node
was modified to add one more state variable, nearest_neighbor_Kminus.
It is not stored in the node, just computed on the fly when
requested. To return nearest_neighbor_Kminus I changed the
archiving_node.get_K_values() function signature, because, as fas as
I can see, this function is not currently used anywhere.

The tests for all three models reside in a single file (but as three
separate tests), and are the following: generate two Poisson spike
sequences as presynaptic and postsynaptic, feed them to NEST to get
the synaptic weight change, then reproduce the weight change
independently outside of NEST and check whether the expected weight
equals the obtained one.

aserenko added a commit to aserenko/nest-simulator that referenced this pull request Dec 6, 2017

Add STDP synapses with nearest-neighbor spike pairing schemes
Add three types of nearest-neighbour STDP: symmetric (but not to be
confused with PR #218), presynaptic-centered, and restricted
symmetric. These are worth implementing because described in a highly-
cited review [Morrison A., Diesmann M., and Gerstner W. (2008)
Phenomenological models of synaptic plasticity based on spike timing,
Biol. Cybern. 98, 459--478].

In these models a spike is taken into account in the weight change
rule not with all preceding spikes, but only with some of the
nearest, see the three pairing schemes in Fig. 7 in [Morrison et
al., 2008].

The implementation relies on two additional variables - presynaptic
and postsynaptic traces, like in stdp_synapse. However, their update
rules differ from those of the latter. That's why the archiving_node
was modified to add one more state variable, nearest_neighbor_Kminus.
It is not stored in the node, just computed on the fly when
requested. To return nearest_neighbor_Kminus I changed the
archiving_node.get_K_values() function signature, because, as fas as
I can see, this function is not currently used anywhere.

The tests for all three models reside in a single file (but as three
separate tests), and are the following: generate two Poisson spike
sequences as presynaptic and postsynaptic, feed them to NEST to get
the synaptic weight change, then reproduce the weight change
independently outside of NEST and check whether the expected weight
equals the obtained one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment