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

Correct input handling for aeif_cond_beta_multisynapse #510

Closed
heplesser opened this Issue Oct 6, 2016 · 17 comments

Comments

3 participants
@heplesser
Contributor

heplesser commented Oct 6, 2016

Input handling for aeif_cond_beta_multisynapse needs to be modified according to the decision made in the NEST Developer VC 19 Sep 2016:

  • Replace E_ex, E_in with vector of reversal potentials the same size as taus_syn (one per synapse)
  • Replace I_syn_ex, I_syn_in recordables with one recordable I_syn representing the total input current
  • In handle(SpikeEvent&), consider adding a check for non-negative weight.
  • Update *_dynamics function to handle input current correctly, along the lines of
  double I_syn = 0.0;
  for ( size_t i = 0; i < node.P_.num_of_receptors_ ; ++i )
  {
      I_syn += y[ S::G + i ] * ( node.P_.E_rev[i] - V );
  }
  • Provide examples (python based).

See also #509, #511.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 6, 2016

Contributor

@golosio Could you look at this? cond_beta is already furthest, since it has the proper handle() method.

Contributor

heplesser commented Oct 6, 2016

@golosio Could you look at this? cond_beta is already furthest, since it has the proper handle() method.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 6, 2016

Contributor

Sure, I'll have a look.

Contributor

golosio commented Oct 6, 2016

Sure, I'll have a look.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 7, 2016

Contributor

We have to take care that the same quantity should have the same name in different models. How should I call the reversal potential? Maybe V_rev (I prefer this) or E_rev?

Contributor

golosio commented Oct 7, 2016

We have to take care that the same quantity should have the same name in different models. How should I call the reversal potential? Maybe V_rev (I prefer this) or E_rev?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 7, 2016

Contributor

I think in the literature reversal potentials are usually given as E, so I would suggest E_rev. @Silmathoron Any thoughts on this?

Contributor

heplesser commented Oct 7, 2016

I think in the literature reversal potentials are usually given as E, so I would suggest E_rev. @Silmathoron Any thoughts on this?

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 7, 2016

Contributor

For conductance-based flows, I've never seen something other than E be used... so E_rev looks good! Unfortunately, here a more complete unification to be coherent with the "unisynapse" models will probably be impossible since people have been using conductance-based models for a long time and the _rev part was never there... but E_rev seems like the better choice anyway.

Contributor

Silmathoron commented Oct 7, 2016

For conductance-based flows, I've never seen something other than E be used... so E_rev looks good! Unfortunately, here a more complete unification to be coherent with the "unisynapse" models will probably be impossible since people have been using conductance-based models for a long time and the _rev part was never there... but E_rev seems like the better choice anyway.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 7, 2016

Contributor

By the way, just a thought since we're at it: should there be a possibility of giving only a double for the reversal potentials and taus if the user want them to be all equal?

Contributor

Silmathoron commented Oct 7, 2016

By the way, just a thought since we're at it: should there be a possibility of giving only a double for the reversal potentials and taus if the user want them to be all equal?

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 7, 2016

Contributor

Ok. Also, we should take a decision on what to do when the number of receptor ports is changed, and when E_rev or taus_syn ( taus_rise or taus_decay for cond_beta ) are defined or redefined with a number of elements that does not match the current number of receptor ports.
In aeif_cond_beta_multisynapse I tried to follow the same approach of aeif_cond_alpha_multisynapse and I used one of the vectors (taus_decay) to control the number of receptor ports. However I thought about this a lot and I think it was not the best approach.
My proposal is:

  • by default at creation the neuron has one receptor port, with default values for E_rev (0.0 mV) and for taus_syn/rise/decay
  • the number of receptor ports can only by changed by changing the value of num_of_receptors_, which therefore must be set properly before assigning the values of E_rev and/or taus_syn/rise/decay
  • num_of_receptors_ cannot be reduced if the neuron already have connections
  • if num_of_receptors_ is increased, the new elements of E_rev and taus_syn/rise/decay are filled with default values, the old elements remain unchanged
  • if the user tries to assign E_rev or taus_syn/rise/decay with a number of elements that does not match num_of_receptors_ the program gives an error.
    Let me know if you agree with those choices.
Contributor

golosio commented Oct 7, 2016

Ok. Also, we should take a decision on what to do when the number of receptor ports is changed, and when E_rev or taus_syn ( taus_rise or taus_decay for cond_beta ) are defined or redefined with a number of elements that does not match the current number of receptor ports.
In aeif_cond_beta_multisynapse I tried to follow the same approach of aeif_cond_alpha_multisynapse and I used one of the vectors (taus_decay) to control the number of receptor ports. However I thought about this a lot and I think it was not the best approach.
My proposal is:

  • by default at creation the neuron has one receptor port, with default values for E_rev (0.0 mV) and for taus_syn/rise/decay
  • the number of receptor ports can only by changed by changing the value of num_of_receptors_, which therefore must be set properly before assigning the values of E_rev and/or taus_syn/rise/decay
  • num_of_receptors_ cannot be reduced if the neuron already have connections
  • if num_of_receptors_ is increased, the new elements of E_rev and taus_syn/rise/decay are filled with default values, the old elements remain unchanged
  • if the user tries to assign E_rev or taus_syn/rise/decay with a number of elements that does not match num_of_receptors_ the program gives an error.
    Let me know if you agree with those choices.
@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 7, 2016

Contributor

This sounds reasonable, but just to be sure: when you say

the number of receptor ports can only by changed by changing the value of num_of_receptors_, which therefore must be set properly before assigning the values of E_rev and/or taus_syn/rise/decay

you mean that nest.SetStatus({"E_rev": Elist}) would raise an error, but not nest.SetStatus({"num_of_receptor": len(Elist), "E_rev": Elist}), right?

And nest.SetStatus({"num_of_receptor": old_number + 1}) would automatically concatenate E_rev and taus_syn_rise/decay with default values?

Contributor

Silmathoron commented Oct 7, 2016

This sounds reasonable, but just to be sure: when you say

the number of receptor ports can only by changed by changing the value of num_of_receptors_, which therefore must be set properly before assigning the values of E_rev and/or taus_syn/rise/decay

you mean that nest.SetStatus({"E_rev": Elist}) would raise an error, but not nest.SetStatus({"num_of_receptor": len(Elist), "E_rev": Elist}), right?

And nest.SetStatus({"num_of_receptor": old_number + 1}) would automatically concatenate E_rev and taus_syn_rise/decay with default values?

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 7, 2016

Contributor

right!

Contributor

golosio commented Oct 7, 2016

right!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 8, 2016

Contributor

@golosio @Silmathoron I would suggest that we force the user to be explicit: if a call to SetStatus contains taus_syn, it also must contain E_rev, and they must be of the same length; num_receptors is a read-only variable. taus_syn and E_rev cannot shrink once connections exist. I would even consider creating the neurons with empty taus_syn and E_rev (implying num_receptors == 0) , because there are no really sensible default values: the multisynapse variants are there to give users full flexibility.

Contributor

heplesser commented Oct 8, 2016

@golosio @Silmathoron I would suggest that we force the user to be explicit: if a call to SetStatus contains taus_syn, it also must contain E_rev, and they must be of the same length; num_receptors is a read-only variable. taus_syn and E_rev cannot shrink once connections exist. I would even consider creating the neurons with empty taus_syn and E_rev (implying num_receptors == 0) , because there are no really sensible default values: the multisynapse variants are there to give users full flexibility.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 8, 2016

Contributor

Ok, I'll try to do it in this way, however I'm not sure that I can check if
a call to SetStatus contains both synaptic time constants and reversal
potential from the model code itself, without modifying other parts of nest
code.
Let me know if you already have an idea of how to check it.

2016-10-08 6:43 GMT+02:00 Hans Ekkehard Plesser notifications@github.com:

@golosio https://github.com/golosio @Silmathoron
https://github.com/Silmathoron I would suggest that we force the user
to be explicit: if a call to SetStatus contains taus_syn, it also must
contain E_rev, and they must be of the same length; num_receptors is a
read-only variable. taus_syn and E_rev cannot shrink once connections
exist. I would even consider creating the neurons with empty taus_syn and
E_rev (implying num_receptors == 0) , because there are no really
sensible default values: the multisynapse variants are there to give users
full flexibility.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#510 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADrHOWOFbW-OpeuZWVhlNI6tuySYbCaiks5qxx-DgaJpZM4KPrPq
.

Contributor

golosio commented Oct 8, 2016

Ok, I'll try to do it in this way, however I'm not sure that I can check if
a call to SetStatus contains both synaptic time constants and reversal
potential from the model code itself, without modifying other parts of nest
code.
Let me know if you already have an idea of how to check it.

2016-10-08 6:43 GMT+02:00 Hans Ekkehard Plesser notifications@github.com:

@golosio https://github.com/golosio @Silmathoron
https://github.com/Silmathoron I would suggest that we force the user
to be explicit: if a call to SetStatus contains taus_syn, it also must
contain E_rev, and they must be of the same length; num_receptors is a
read-only variable. taus_syn and E_rev cannot shrink once connections
exist. I would even consider creating the neurons with empty taus_syn and
E_rev (implying num_receptors == 0) , because there are no really
sensible default values: the multisynapse variants are there to give users
full flexibility.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#510 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADrHOWOFbW-OpeuZWVhlNI6tuySYbCaiks5qxx-DgaJpZM4KPrPq
.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 9, 2016

Contributor

@heplesser @Silmathoron ok I checked and it should not be difficult to make the checks inside the model code without involving other parts of the code. Sorry, before I did not have clear how multiple DictionaryDatum were processed. I'll try to implement the solution proposed by @heplesser .

Contributor

golosio commented Oct 9, 2016

@heplesser @Silmathoron ok I checked and it should not be difficult to make the checks inside the model code without involving other parts of the code. Sorry, before I did not have clear how multiple DictionaryDatum were processed. I'll try to implement the solution proposed by @heplesser .

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 9, 2016

Contributor

@heplesser

I would even consider creating the neurons with empty taus_syn and E_rev (implying num_receptors == 0)

I am not sure about this... I liked the idea that if the user creates a multisynapse neuron without explicitly setting its parameters and just use it, it will behave as a single-receptor-port neuron, rather than giving an error...

Contributor

golosio commented Oct 9, 2016

@heplesser

I would even consider creating the neurons with empty taus_syn and E_rev (implying num_receptors == 0)

I am not sure about this... I liked the idea that if the user creates a multisynapse neuron without explicitly setting its parameters and just use it, it will behave as a single-receptor-port neuron, rather than giving an error...

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 9, 2016

Contributor

@golosio We may want to discuss this in the next Developer VC on Oct 17.

Contributor

heplesser commented Oct 9, 2016

@golosio We may want to discuss this in the next Developer VC on Oct 17.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 9, 2016

Contributor

@heplesser I found a solution that fulfill your suggestions by changing
only the model code, see my PR.

Il 09/Ott/2016 11:55, "Hans Ekkehard Plesser" notifications@github.com ha
scritto:

@golosio https://github.com/golosio We may want to discuss this in the
next Developer VC on Oct 17.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#510 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADrHOcq54ERgnngv_RYWwGXc1BZncEVQks5qyLn_gaJpZM4KPrPq
.

Contributor

golosio commented Oct 9, 2016

@heplesser I found a solution that fulfill your suggestions by changing
only the model code, see my PR.

Il 09/Ott/2016 11:55, "Hans Ekkehard Plesser" notifications@github.com ha
scritto:

@golosio https://github.com/golosio We may want to discuss this in the
next Developer VC on Oct 17.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#510 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADrHOcq54ERgnngv_RYWwGXc1BZncEVQks5qyLn_gaJpZM4KPrPq
.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Oct 9, 2016

Contributor

Ok, I'll try to have a look at the PR this week, but I guess we should wait for the VC to make a decision on the default behaviour (though I agree with @golosio that forcing the user to be explicit might be seen as obnoxious from his -- the user's -- point of view, even though we think we're doing it for his own good ^^).

Contributor

Silmathoron commented Oct 9, 2016

Ok, I'll try to have a look at the PR this week, but I guess we should wait for the VC to make a decision on the default behaviour (though I agree with @golosio that forcing the user to be explicit might be seen as obnoxious from his -- the user's -- point of view, even though we think we're doing it for his own good ^^).

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Oct 10, 2016

Contributor

As I will not be able to participate to the VC, I want to say that I do not have a strong opinion about the default behavior. For sure @heplesser has much more experience then me on how to handle interaction with the user.

Contributor

golosio commented Oct 10, 2016

As I will not be able to participate to the VC, I want to say that I do not have a strong opinion about the default behavior. For sure @heplesser has much more experience then me on how to handle interaction with the user.

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