nest / nest-simulator

The NEST simulator
http://www.nest-simulator.org
GNU General Public License v2.0
541 stars 367 forks source link

Correct input handling for aeif_cond_beta_multisynapse #510

Closed heplesser closed 8 years ago

heplesser commented 8 years ago

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

  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 );
  }

See also #509, #511.

heplesser commented 8 years ago

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

golosio commented 8 years ago

Sure, I'll have a look.

golosio commented 8 years ago

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 commented 8 years ago

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

Silmathoron commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 _condbeta ) 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:

Silmathoron commented 8 years ago

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 commented 8 years ago

right!

heplesser commented 8 years ago

@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 commented 8 years ago

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 https://github.com/nest/nest-simulator/issues/510#issuecomment-252402610, or mute the thread https://github.com/notifications/unsubscribe-auth/ADrHOWOFbW-OpeuZWVhlNI6tuySYbCaiks5qxx-DgaJpZM4KPrPq .

golosio commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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

golosio commented 8 years ago

@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 https://github.com/nest/nest-simulator/issues/510#issuecomment-252476825, or mute the thread https://github.com/notifications/unsubscribe-auth/ADrHOcq54ERgnngv_RYWwGXc1BZncEVQks5qyLn_gaJpZM4KPrPq .

Silmathoron commented 8 years ago

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 commented 8 years ago

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.