Changed order of setting parameters for synapse models #333

Merged
merged 1 commit into from May 31, 2016

Conversation

Projects
None yet
4 participants
@weidel-p
Contributor

weidel-p commented May 4, 2016

In #284, I modified the stdp synapse model to allow inhibitory synapses (weight with negative signs). Also, I wanted to throw an exception if the parameters weight and Wmax don't have the same sign.

While creating a new connection the parameter weight is set in the function set_weight and parameters like Wmax are set in set_status. In the current implementation, set_status is called before set_weight, which has the effect that weight is not yet updated in set_status.

For asserting that weight and Wmax has the same sign in set_status, I had to change the order in which set_weight and set_status is called. Now, set_status is called after set_weight (and set_delay) which ensures all parameters are up to date in set_status.

@weidel-p weidel-p changed the title from changed order of setting parameters in connection to Changed order of setting parameters for synapse models May 4, 2016

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm May 4, 2016

Contributor

Looks fine to me 👍

Contributor

abigailm commented May 4, 2016

Looks fine to me 👍

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 4, 2016

Contributor

@weidel-p It would be very nice to have a regression test showing the problem you solve here. Just to make sure that we do not reintroduce it later.

I am also wondering a little about the logic here. Wmax is a parameter, weight is part of the state of the synapse. In general, parameters should define conditions on state variables, not the other way around. In principal, this would mean that set_weight() should check the weight agains Wmax. But that might be very difficult (and possibly inefficient) to implement since set_weight() applies to all synapse models, while Wmax is more specific.

Contributor

heplesser commented May 4, 2016

@weidel-p It would be very nice to have a regression test showing the problem you solve here. Just to make sure that we do not reintroduce it later.

I am also wondering a little about the logic here. Wmax is a parameter, weight is part of the state of the synapse. In general, parameters should define conditions on state variables, not the other way around. In principal, this would mean that set_weight() should check the weight agains Wmax. But that might be very difficult (and possibly inefficient) to implement since set_weight() applies to all synapse models, while Wmax is more specific.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p May 4, 2016

Contributor

@heplesser as I metioned above, in #284, I changed the STDPConnection and STDPTripletConnection to allow negative weights. I was asked to throw an exception if the parameters weight and Wmax are not the same sign instead of solving that under the hood.

There are three different ways to set parameters of a synapse:

  1. By using SetDefaults on the connection type.
  2. By using Create and setting the syn_spec.
  3. By using SetStatus after the connection was created.

Please correct me if I'm wrong, but from these three options, only option 2 calls set_weight AND set_status. Option 1 and 3 only call set_status. So, I agree with you, I'm also wondering about the logic here, but I don't know enough about this topic.

Anyway, in #284 I want to check for correct signs in all three cases which was not possible so far. I think we don't need a dedicated test to make sure that this issue is not reappearing. As soon as #284 is merged, the tests for the STDPConnection cover this issue.

Contributor

weidel-p commented May 4, 2016

@heplesser as I metioned above, in #284, I changed the STDPConnection and STDPTripletConnection to allow negative weights. I was asked to throw an exception if the parameters weight and Wmax are not the same sign instead of solving that under the hood.

There are three different ways to set parameters of a synapse:

  1. By using SetDefaults on the connection type.
  2. By using Create and setting the syn_spec.
  3. By using SetStatus after the connection was created.

Please correct me if I'm wrong, but from these three options, only option 2 calls set_weight AND set_status. Option 1 and 3 only call set_status. So, I agree with you, I'm also wondering about the logic here, but I don't know enough about this topic.

Anyway, in #284 I want to check for correct signs in all three cases which was not possible so far. I think we don't need a dedicated test to make sure that this issue is not reappearing. As soon as #284 is merged, the tests for the STDPConnection cover this issue.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 9, 2016

Contributor

@weidel-p Taking the last issue first (test): Do some of the STDPConnection tests fail at the moment because of the wrong order? If yes, fine. If no, we should add an additional test.

I will get back to the order, but need to catch a flight first.

Contributor

heplesser commented May 9, 2016

@weidel-p Taking the last issue first (test): Do some of the STDPConnection tests fail at the moment because of the wrong order? If yes, fine. If no, we should add an additional test.

I will get back to the order, but need to catch a flight first.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 9, 2016

Contributor

@weidel-p I agree with your analysis of who calls set_weight(), but note that the function is called from two places in connector_model_impl.h.

I think we need to implement checking the sign of the weight both in set_weight() and in set_status() to cover all possible cases (NB: tests should be applied to all synapses having /Wmax as parameter, or?):

{
  ResetKernel
  /stdp_synapse << /weight 1. / Wmax 2. >>  SetDefaults
} pass_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. / Wmax -2. >>  SetDefaults
} pass_or_die

{
  ResetKernel
  /stdp_synapse << /weight -3. / Wmax -2. >>  SetDefaults
} fail_or_die

{
 ResetKernel
  /stdp_synapse << / Wmax -2. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. / Wmax -2. >>  SetDefaults
  /stdp_synapse << /weight 1. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. / Wmax -2. >>  SetDefaults
  /stdp_synapse << /Wmax 2. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /iaf_psc_alpha Create
  dup 1.0 2.0 /stdp_synapse Connect
} pass_or_die

{
  ResetKernel
  /iaf_psc_alpha Create
  dup -1.0 2.0 /stdp_synapse Connect
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -0.5 /Wmax -2.0 >> SetDefaults
  /iaf_psc_alpha Create
  dup -1.0 2.0 /stdp_synapse Connect
} pass_or_die

{
  ResetKernel
  /iaf_psc_alpha Create 1 arraystore
  dup /one_to_one << /model /stdp_synapse /weight -1.0 >> Connect
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -0.5 /Wmax -2.0 >> SetDefaults
  /iaf_psc_alpha Create 1 arraystore
  dup /one_to_one << /model /stdp_synapse /weight -1.0 >> Connect
} pass_or_die

{
  ResetKernel
  /iaf_psc_alpha Create 1 arraystore
  dup /one_to_one << /model /stdp_synapse /weight -1.0  /Wmax -2.0 >> Connect
} pass_or_die

{
  ResetKernel
 /stdp_synapse << /Wmax 20. >> SetDefaults
  /iaf_psc_alpha Create 1 arraystore
 dup 
 << /rule /fixed_indegree /indegree 10 >> 
 << /weight [ 10 ] { neg } Table cv_dv 
    /model /stdp_synapse >> 
 Connect
} fail_or_die

{
  ResetKernel
 /stdp_synapse << /Wmax -20. >> SetDefaults
  /iaf_psc_alpha Create 1 arraystore
 dup 
 << /rule /fixed_indegree /indegree 10 >> 
 << /weight [ 10 ] { neg } Table cv_dv 
    /model /stdp_synapse >> 
 Connect
} pass_or_die

Therefore,

  • set_status() must check that w and Wmax are consistent
  • set_weight() must check that the new weight is consistent with Wmax; this must also happen if set_status() is not called
  • If a parameter dictionary, which may contain Wmax, and an explicit weight are passed, then we must
    1. set the new Wmax via set_status, but tell set_status() that is should not check w-Wmax compatibility; we can do this by passing not numerics::is_nan(weight) to set_status() as an optional argument skip_Wmax_check with default valuefalse.
    2. call set_weight() to set the new weight, checking against the updated Wmax value
Contributor

heplesser commented May 9, 2016

@weidel-p I agree with your analysis of who calls set_weight(), but note that the function is called from two places in connector_model_impl.h.

I think we need to implement checking the sign of the weight both in set_weight() and in set_status() to cover all possible cases (NB: tests should be applied to all synapses having /Wmax as parameter, or?):

{
  ResetKernel
  /stdp_synapse << /weight 1. / Wmax 2. >>  SetDefaults
} pass_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. / Wmax -2. >>  SetDefaults
} pass_or_die

{
  ResetKernel
  /stdp_synapse << /weight -3. / Wmax -2. >>  SetDefaults
} fail_or_die

{
 ResetKernel
  /stdp_synapse << / Wmax -2. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. / Wmax -2. >>  SetDefaults
  /stdp_synapse << /weight 1. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -1. / Wmax -2. >>  SetDefaults
  /stdp_synapse << /Wmax 2. >> SetDefaults
} fail_or_die

{
  ResetKernel
  /iaf_psc_alpha Create
  dup 1.0 2.0 /stdp_synapse Connect
} pass_or_die

{
  ResetKernel
  /iaf_psc_alpha Create
  dup -1.0 2.0 /stdp_synapse Connect
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -0.5 /Wmax -2.0 >> SetDefaults
  /iaf_psc_alpha Create
  dup -1.0 2.0 /stdp_synapse Connect
} pass_or_die

{
  ResetKernel
  /iaf_psc_alpha Create 1 arraystore
  dup /one_to_one << /model /stdp_synapse /weight -1.0 >> Connect
} fail_or_die

{
  ResetKernel
  /stdp_synapse << /weight -0.5 /Wmax -2.0 >> SetDefaults
  /iaf_psc_alpha Create 1 arraystore
  dup /one_to_one << /model /stdp_synapse /weight -1.0 >> Connect
} pass_or_die

{
  ResetKernel
  /iaf_psc_alpha Create 1 arraystore
  dup /one_to_one << /model /stdp_synapse /weight -1.0  /Wmax -2.0 >> Connect
} pass_or_die

{
  ResetKernel
 /stdp_synapse << /Wmax 20. >> SetDefaults
  /iaf_psc_alpha Create 1 arraystore
 dup 
 << /rule /fixed_indegree /indegree 10 >> 
 << /weight [ 10 ] { neg } Table cv_dv 
    /model /stdp_synapse >> 
 Connect
} fail_or_die

{
  ResetKernel
 /stdp_synapse << /Wmax -20. >> SetDefaults
  /iaf_psc_alpha Create 1 arraystore
 dup 
 << /rule /fixed_indegree /indegree 10 >> 
 << /weight [ 10 ] { neg } Table cv_dv 
    /model /stdp_synapse >> 
 Connect
} pass_or_die

Therefore,

  • set_status() must check that w and Wmax are consistent
  • set_weight() must check that the new weight is consistent with Wmax; this must also happen if set_status() is not called
  • If a parameter dictionary, which may contain Wmax, and an explicit weight are passed, then we must
    1. set the new Wmax via set_status, but tell set_status() that is should not check w-Wmax compatibility; we can do this by passing not numerics::is_nan(weight) to set_status() as an optional argument skip_Wmax_check with default valuefalse.
    2. call set_weight() to set the new weight, checking against the updated Wmax value
@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p May 25, 2016

Contributor

@heplesser yes, the in #284 a test is failing because of the wrong order.

I think the tests for the signs of Wmax and weight should be part of #284, where I already implemented a test. As soon as this PR is merged, I can proceed to work on that.

Contributor

weidel-p commented May 25, 2016

@heplesser yes, the in #284 a test is failing because of the wrong order.

I think the tests for the signs of Wmax and weight should be part of #284, where I already implemented a test. As soon as this PR is merged, I can proceed to work on that.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 25, 2016

Contributor

@weidel-p Sounds like a fine plan.

Contributor

heplesser commented May 25, 2016

@weidel-p Sounds like a fine plan.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen May 31, 2016

Contributor

@heplesser Does this mean you give your plus-1 for this (#333) PR, so that @weidel-p can work on #284 and implement the test there?

Contributor

tammoippen commented May 31, 2016

@heplesser Does this mean you give your plus-1 for this (#333) PR, so that @weidel-p can work on #284 and implement the test there?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 31, 2016

Contributor

@tammoippen @weidel-p Yes, 👍 and merging.

Contributor

heplesser commented May 31, 2016

@tammoippen @weidel-p Yes, 👍 and merging.

@heplesser heplesser merged commit 9faa71e into nest:master May 31, 2016

1 check passed

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

@weidel-p weidel-p referenced this pull request Jun 12, 2016

Merged

Enables inhibitory stdp #284

jakobj pushed a commit to jakobj/nest-simulator that referenced this pull request Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment