Enables inhibitory stdp #284

Merged
merged 17 commits into from Jun 14, 2016

Conversation

Projects
None yet
6 participants
@weidel-p
Contributor

weidel-p commented Mar 23, 2016

With this pull request, STDP is possible for inhibitory connections by assigning negative values for weight and Wmax in STDPConnection and STDPTripletConnection.
The functions facilitate and depress assumes positive weight changes, but the actual sign of the weight change is applied in the send function.
The sign of weight determines the sign of Wmax during connection and SetStatus. During simulation, the sign of Wmax determines the sign of weight.

This addresses #240.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Mar 23, 2016

Contributor

I suggest @abigailm, @mhelias and/or @suku248 as reviewers.

Contributor

weidel-p commented Mar 23, 2016

I suggest @abigailm, @mhelias and/or @suku248 as reviewers.

models/stdp_connection.h
@@ -185,7 +187,7 @@ class STDPConnection : public Connection< targetidentifierT >
{
double_t norm_w =
( w / Wmax_ ) - ( alpha_ * lambda_ * std::pow( w / Wmax_, mu_minus_ ) * kminus );
- return norm_w > 0.0 ? norm_w * Wmax_ : 0.0;
+ return norm_w > 0.0 ? norm_w * Wmax_ : copysign( 0.0, Wmax_ );

This comment has been minimized.

@heplesser

heplesser Mar 24, 2016

Contributor

copysign( 0.0, Wmax_ ) returns a value with the magnitude of its first and the sign of its second argument. So here we would get +0 og -0. Does that make sense?

@heplesser

heplesser Mar 24, 2016

Contributor

copysign( 0.0, Wmax_ ) returns a value with the magnitude of its first and the sign of its second argument. So here we would get +0 og -0. Does that make sense?

This comment has been minimized.

@weidel-p

weidel-p Mar 24, 2016

Contributor

With this line, I wanted to make sure that the sign of weight does not flip from negative to positive if it becomes 0. After having another look into this and after testing, I agree that it was unnecessary.
I fixed that in 0ebe4ba.

@weidel-p

weidel-p Mar 24, 2016

Contributor

With this line, I wanted to make sure that the sign of weight does not flip from negative to positive if it becomes 0. After having another look into this and after testing, I agree that it was unnecessary.
I fixed that in 0ebe4ba.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 24, 2016

Contributor

@weidel-p The copysign() function is part of C++11. Since we cannot rely on C++11 support on all relevant compilers, we need a configure test for whether copysign() is available and if not, we need to define it ourselves, e.g., in libnestutil.numerics.h.

Contributor

heplesser commented Mar 24, 2016

@weidel-p The copysign() function is part of C++11. Since we cannot rely on C++11 support on all relevant compilers, we need a configure test for whether copysign() is available and if not, we need to define it ourselves, e.g., in libnestutil.numerics.h.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Mar 24, 2016

Contributor

@heplesser copysign() is also available in C99. In 0ebe4ba I changed the include to use the C header. Is this already sufficient to make sure all relevant compilers support this or shall I include the function to libnestutil.numerics.h as you suggested?

Contributor

weidel-p commented Mar 24, 2016

@heplesser copysign() is also available in C99. In 0ebe4ba I changed the include to use the C header. Is this already sufficient to make sure all relevant compilers support this or shall I include the function to libnestutil.numerics.h as you suggested?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 25, 2016

Contributor

@weidel-p I tried

#include <math.h>
#include <iostream>

int main()
{
  double x = 10;
  double y = -2;

  double z = copysign(x, y);

  std::cout << x << '\t' << y << '\t'  << z << std::endl;

  return 0;
}

using xlc++, Intel, Portland and Clang compilers. It works everywhere. The only system I cannot test is K. Could you ask our K-colleagues to test it?

Contributor

heplesser commented Mar 25, 2016

@weidel-p I tried

#include <math.h>
#include <iostream>

int main()
{
  double x = 10;
  double y = -2;

  double z = copysign(x, y);

  std::cout << x << '\t' << y << '\t'  << z << std::endl;

  return 0;
}

using xlc++, Intel, Portland and Clang compilers. It works everywhere. The only system I cannot test is K. Could you ask our K-colleagues to test it?

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Mar 26, 2016

Contributor

with FCCpx and mpiFCCpx @heplesser's script works also on K. no compiler warnings/errors and the output reads 10 -2 -10 which is correct as far as I can see.

Contributor

jakobj commented Mar 26, 2016

with FCCpx and mpiFCCpx @heplesser's script works also on K. no compiler warnings/errors and the output reads 10 -2 -10 which is correct as far as I can see.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 28, 2016

Contributor

@jakobj Thanks for checking on K!
@weidel-p Could you make a tiny, inconsequential change to trigger a new build? The last build failed partially due to time-outs. Conditional on all test passing 👍 from my side, but one of the plasticity experts should approve before merging.

Contributor

heplesser commented Mar 28, 2016

@jakobj Thanks for checking on K!
@weidel-p Could you make a tiny, inconsequential change to trigger a new build? The last build failed partially due to time-outs. Conditional on all test passing 👍 from my side, but one of the plasticity experts should approve before merging.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Apr 5, 2016

Contributor

Thanks for this fix - just a thought - is using copysign to copy the sign of w to wmax under the hood a good idea? Wouldn't it be better to have a parameter check that errors out and makes the user explicitly set both signs - so that the user is fully aware of the nature of the synapse?

This way, if folks use pynest for example, both w and wmax will be given negative values in the python script, which is a lot more sensible and intuitive than having them with different signs - which just looks wrong and doesn't fit the "negative weights imply inhibitory synapses" convention. Also, consider the scenario where a user misses out a negative sign in w but gives a negative wmax correctly with the intention of having an inhibitory synapse - the negative sign in wmax gets overridden under the hood and the user doesn't realise the error until the simulation has been run and some unexpected results received.

The sign of a synapse, and therefore its nature (E or I), should not change mid simulation anyway - this isn't biologically plausible, is it? If it isn't, there shouldn't be a copysign in the set weight method - there should be an error check instead.

Lastly, can we document the negative weights convention somewhere - preferably with an example? Maybe in the connection management documentation? (I'll be happy to do this and open a pull request once this one is merged)

(I'll update my vogels-sprekeler implementation in #218 to use the same system once this request is merged - at the moment, that needs a negative eta value and the checks are implemented in a different way)

Contributor

sanjayankur31 commented Apr 5, 2016

Thanks for this fix - just a thought - is using copysign to copy the sign of w to wmax under the hood a good idea? Wouldn't it be better to have a parameter check that errors out and makes the user explicitly set both signs - so that the user is fully aware of the nature of the synapse?

This way, if folks use pynest for example, both w and wmax will be given negative values in the python script, which is a lot more sensible and intuitive than having them with different signs - which just looks wrong and doesn't fit the "negative weights imply inhibitory synapses" convention. Also, consider the scenario where a user misses out a negative sign in w but gives a negative wmax correctly with the intention of having an inhibitory synapse - the negative sign in wmax gets overridden under the hood and the user doesn't realise the error until the simulation has been run and some unexpected results received.

The sign of a synapse, and therefore its nature (E or I), should not change mid simulation anyway - this isn't biologically plausible, is it? If it isn't, there shouldn't be a copysign in the set weight method - there should be an error check instead.

Lastly, can we document the negative weights convention somewhere - preferably with an example? Maybe in the connection management documentation? (I'll be happy to do this and open a pull request once this one is merged)

(I'll update my vogels-sprekeler implementation in #218 to use the same system once this request is merged - at the moment, that needs a negative eta value and the checks are implemented in a different way)

models/stdp_triplet_connection.h
@@ -185,7 +187,7 @@ class STDPTripletConnection : public Connection< targetidentifierT >
facilitate_( double_t w, double_t kplus, double_t ky )
{
double_t new_w = w + kplus * ( Aplus_ + Aplus_triplet_ * ky );
- return new_w < Wmax_ ? new_w : Wmax_;
+ return new_w < std::abs( Wmax_ ) ? new_w : std::abs( Wmax_ );

This comment has been minimized.

@heplesser

heplesser Apr 5, 2016

Contributor

Looking at this code again after a comment from @sanjayankur31 which is strangely missing from Github, I find the code very difficult to read. The problem is that on l. 254f you call call facilitate_() with the absolute value of w, something that is not clear from the code here. Basically, you mix handling of signs in send() and facilitate_(). I would suggest to do all signe handling inside facilitate_(), so that the code in send() would again become

weight_ = facilitate_( weight_, Kplus_ * std::exp( minus_dt / tau_plus_ ), ky );

That way, all sign handling will be inside facilitate_(), which we then could code like this

const double_t new_w_abs = std::abs( w ) + kplus * ( Aplus_ + Aplus_triplet_ * ky );
return copysign( std::min( new_w_abs, std::abs( Wmax_ ) ), Wmax_ );

I suppose the same applies to depress_().

@heplesser

heplesser Apr 5, 2016

Contributor

Looking at this code again after a comment from @sanjayankur31 which is strangely missing from Github, I find the code very difficult to read. The problem is that on l. 254f you call call facilitate_() with the absolute value of w, something that is not clear from the code here. Basically, you mix handling of signs in send() and facilitate_(). I would suggest to do all signe handling inside facilitate_(), so that the code in send() would again become

weight_ = facilitate_( weight_, Kplus_ * std::exp( minus_dt / tau_plus_ ), ky );

That way, all sign handling will be inside facilitate_(), which we then could code like this

const double_t new_w_abs = std::abs( w ) + kplus * ( Aplus_ + Aplus_triplet_ * ky );
return copysign( std::min( new_w_abs, std::abs( Wmax_ ) ), Wmax_ );

I suppose the same applies to depress_().

This comment has been minimized.

@sanjayankur31

sanjayankur31 Apr 5, 2016

Contributor

Oh, sorry - I removed the comment - I wasn't quite sure if what I was asking was correct - hadn't worked it out on paper yet.

@sanjayankur31

sanjayankur31 Apr 5, 2016

Contributor

Oh, sorry - I removed the comment - I wasn't quite sure if what I was asking was correct - hadn't worked it out on paper yet.

This comment has been minimized.

@flinz

flinz Apr 18, 2016

Contributor

@weidel-p I agree with @heplesser and @sanjayankur31 that the handling of the sign issues for weight updates should be put into facilitate and depress respectively.

@flinz

flinz Apr 18, 2016

Contributor

@weidel-p I agree with @heplesser and @sanjayankur31 that the handling of the sign issues for weight updates should be put into facilitate and depress respectively.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 5, 2016

Contributor

@sanjayankur31 I suppose in your comment on changing the sign under the hood you refer to STDPConnection::set_weight() and STDPConnection::set_status(). I agree that it would make sense to introduce a test in both places, raising an exception if one tries to set a weight (or weight limit) that is not consistent with the sign. Zero should in this case work for both signs. There should also be a test ensuring that this check is in place.

This would stop users from subtle and hard-to-find errors.

Contributor

heplesser commented Apr 5, 2016

@sanjayankur31 I suppose in your comment on changing the sign under the hood you refer to STDPConnection::set_weight() and STDPConnection::set_status(). I agree that it would make sense to introduce a test in both places, raising an exception if one tries to set a weight (or weight limit) that is not consistent with the sign. Zero should in this case work for both signs. There should also be a test ensuring that this check is in place.

This would stop users from subtle and hard-to-find errors.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Apr 18, 2016

Contributor

@flinz Can you please look into this PR? Thank you.

Contributor

tammoippen commented Apr 18, 2016

@flinz Can you please look into this PR? Thank you.

@@ -84,11 +85,11 @@ def decay(self, time, Kplus, Kplus_triplet, Kminus, Kminus_triplet):
def facilitate(self, w, Kplus, Kminus_triplet):
"""Facilitate weight."""
- return w + Kplus * (self.syn_spec["Aplus"] + self.syn_spec["Aplus_triplet"] * Kminus_triplet)
+ return abs(w + Kplus * (self.syn_spec["Aplus"] + self.syn_spec["Aplus_triplet"] * Kminus_triplet))

This comment has been minimized.

@flinz

flinz Apr 18, 2016

Contributor

After incorporating the changes proposed by @heplesser in #284 (comment) the test functions for facilitate and depress, and calls to them, should be adapted accordingly.

@flinz

flinz Apr 18, 2016

Contributor

After incorporating the changes proposed by @heplesser in #284 (comment) the test functions for facilitate and depress, and calls to them, should be adapted accordingly.

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Apr 18, 2016

Contributor

After the line comments are addressed, 👍 from me.

Contributor

flinz commented Apr 18, 2016

After the line comments are addressed, 👍 from me.

weidel-p added some commits Apr 19, 2016

Merge branch 'master' into asymmetric_stdp
Conflicts:
	models/stdp_triplet_connection.h
	pynest/nest/tests/test_stdp_triplet_synapse.py
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 10, 2016

Contributor

@weidel-p #391 is merged and Travis works again. Could you merge all changes from master into your PR so that Travis hopefully turns green again?

Contributor

heplesser commented Jun 10, 2016

@weidel-p #391 is merged and Travis works again. Could you merge all changes from master into your PR so that Travis hopefully turns green again?

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Jun 12, 2016

Contributor

Hopefully I addressed all comments with the latest commit.

  1. The signs are now exclusively handled in the facilitate and depress functions.
  2. An exception is thrown if Wmax and weight are initialized with different signs. This ensures that users get an immediate feedback if weight and Wmax are set inconsistently.
  3. If the issue solved by #333 is reintroduced again, the tests for #284 will fail.
Contributor

weidel-p commented Jun 12, 2016

Hopefully I addressed all comments with the latest commit.

  1. The signs are now exclusively handled in the facilitate and depress functions.
  2. An exception is thrown if Wmax and weight are initialized with different signs. This ensures that users get an immediate feedback if weight and Wmax are set inconsistently.
  3. If the issue solved by #333 is reintroduced again, the tests for #284 will fail.
models/stdp_connection.h
SeeAlso: synapsedict, tsodyks_synapse, static_synapse
*/
// C++ includes:
-#include <cmath>
+#include <math.h>

This comment has been minimized.

@heplesser

heplesser Jun 12, 2016

Contributor

Comment why you include math.h.

@heplesser

heplesser Jun 12, 2016

Contributor

Comment why you include math.h.

This comment has been minimized.

@weidel-p

weidel-p Jun 14, 2016

Contributor

We were afraid that copysign is not available in the c++ header for some supercomputers. Therefore, I changed it to the C header and we tested this for several compilers. Please read the comments above.

@weidel-p

weidel-p Jun 14, 2016

Contributor

We were afraid that copysign is not available in the c++ header for some supercomputers. Therefore, I changed it to the C header and we tested this for several compilers. Please read the comments above.

This comment has been minimized.

@heplesser

heplesser Jun 14, 2016

Contributor

@weidel-p Yes, I know. But that information should be here in the C++ file, so that if someone reads this file five years from now will understand. E.g. // C-header since copysign() is in C99 but not C++98.

@heplesser

heplesser Jun 14, 2016

Contributor

@weidel-p Yes, I know. But that information should be here in the C++ file, so that if someone reads this file five years from now will understand. E.g. // C-header since copysign() is in C99 but not C++98.

This comment has been minimized.

@weidel-p

weidel-p Jun 14, 2016

Contributor

Right, I added this comment to the cpp file in the stdp_triplet_connection.h.

@weidel-p

weidel-p Jun 14, 2016

Contributor

Right, I added this comment to the cpp file in the stdp_triplet_connection.h.

models/stdp_triplet_connection.h
SeeAlso: stdp_triplet_synapse_hpc, synapsedict, stdp_synapse, static_synapse
*/
-#include <cmath>
+#include <math.h>

This comment has been minimized.

@heplesser

heplesser Jun 12, 2016

Contributor

Comment why you include math.h.

@heplesser

heplesser Jun 12, 2016

Contributor

Comment why you include math.h.

This comment has been minimized.

@weidel-p

weidel-p Jun 14, 2016

Contributor

see above.

@weidel-p

weidel-p Jun 14, 2016

Contributor

see above.

+}
+loop
+
+%final_weight w w_max mul sub abs dup = 1e-13 leq =

This comment has been minimized.

@heplesser

heplesser Jun 12, 2016

Contributor

Remove this commented-out line.

@heplesser

heplesser Jun 12, 2016

Contributor

Remove this commented-out line.

+loop
+
+%final_weight w w_max mul sub abs dup = 1e-13 leq =
+final_weight w w_max mul sub abs 1e-13 leq assert_or_die

This comment has been minimized.

@heplesser

heplesser Jun 12, 2016

Contributor

It would be more in keeping the testsuite style to put the entire loop from l 185 on up to and including the leq into a block and the place the assert_or_die after the block. Also, there is a SLI function ToUnittestPrecision (or similar) that allows testing with a prescribed number of decimals.

@heplesser

heplesser Jun 12, 2016

Contributor

It would be more in keeping the testsuite style to put the entire loop from l 185 on up to and including the leq into a block and the place the assert_or_die after the block. Also, there is a SLI function ToUnittestPrecision (or similar) that allows testing with a prescribed number of decimals.

This comment has been minimized.

@weidel-p

weidel-p Jun 14, 2016

Contributor

This test is basically copied from test_stdp_synapse and adapted for negative signs. Therefore, I tried to modify both tests according to your comments. I am now using ToUnittestPrecision and used a block for the loop. You didn't specify where to put the loop, so I left it where it is.

@weidel-p

weidel-p Jun 14, 2016

Contributor

This test is basically copied from test_stdp_synapse and adapted for negative signs. Therefore, I tried to modify both tests according to your comments. I am now using ToUnittestPrecision and used a block for the loop. You didn't specify where to put the loop, so I left it where it is.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 12, 2016

Contributor

@weidel-p I fear that in all the merging, the sign-handling has disappeared from stdp_connection.h again, at least there is no trace of copysign() left in that file. Could you double-check this?

Contributor

heplesser commented Jun 12, 2016

@weidel-p I fear that in all the merging, the sign-handling has disappeared from stdp_connection.h again, at least there is no trace of copysign() left in that file. Could you double-check this?

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Jun 14, 2016

Contributor

@heplesser in stdp_connection.h no copysign() is needed. In facilitate and depress, the weight is normalized between 0 and 1 and afterwards scaled with Wmax. This works for both, negative and positive weights.
As stated in the initial post, this PR addresses #240. All work done on the stdp_connection.h is just ensuring consistent behavior between the stdp_triplet_synapse and stdp_synapse (throwing the same exception).

Contributor

weidel-p commented Jun 14, 2016

@heplesser in stdp_connection.h no copysign() is needed. In facilitate and depress, the weight is normalized between 0 and 1 and afterwards scaled with Wmax. This works for both, negative and positive weights.
As stated in the initial post, this PR addresses #240. All work done on the stdp_connection.h is just ensuring consistent behavior between the stdp_triplet_synapse and stdp_synapse (throwing the same exception).

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 14, 2016

Contributor

@weidel-p Okay, I overlooked that stdp_connection doesn't need copysign(). But why does it then include math.h instead of cmath?

Contributor

heplesser commented Jun 14, 2016

@weidel-p Okay, I overlooked that stdp_connection doesn't need copysign(). But why does it then include math.h instead of cmath?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 14, 2016

Contributor

@weidel-p The tests look fine now.

Contributor

heplesser commented Jun 14, 2016

@weidel-p The tests look fine now.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Jun 14, 2016

Contributor

@heplesser Sorry, I used copysign() in stdp_connection.h in an earlier version and forgot to change the include back to cmath.

Contributor

weidel-p commented Jun 14, 2016

@heplesser Sorry, I used copysign() in stdp_connection.h in an earlier version and forgot to change the include back to cmath.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 14, 2016

Contributor

👍 and merging. @weidel-p Thanks for you effort!

Contributor

heplesser commented Jun 14, 2016

👍 and merging. @weidel-p Thanks for you effort!

@heplesser heplesser merged commit aa9e6fb into nest:master Jun 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment