Correct input handling for gif_cond_exp_multisynapse (fixes #511) #579

Merged
merged 3 commits into from Dec 9, 2016

Conversation

Projects
None yet
3 participants
@hakonsbm
Contributor

hakonsbm commented Dec 7, 2016

This fixes #511 by, among other things

  • Using a vector of reversal potentials
  • Improving handle and update functions
  • Adding more tests

I suggest @heplesser and @hesam-setareh as reviewers.

@heplesser

@hakonsbm The code is fine, but I have some suggestions for the example in the documentation.

models/gif_cond_exp_multisynapse.h
@@ -130,14 +130,46 @@
Synaptic parameters
tau_syn vector of double - Time constants of the synaptic conductance
in ms.
- E_ex double - Excitatory reversal potential in mV.
- E_in double - Inhibitory reversal potential in mV.
+ E_rev double vector - Reversal potential in mV.

This comment has been minimized.

@heplesser

heplesser Dec 7, 2016

Contributor

For consistency, change type information for tau_syn to double vector, too. In the description of E_rev, it should be plural "potentials". Add (same size as tau_syn) and similar for tau_syn.

@heplesser

heplesser Dec 7, 2016

Contributor

For consistency, change type information for tau_syn to double vector, too. In the description of E_rev, it should be plural "potentials". Add (same size as tau_syn) and similar for tau_syn.

models/gif_cond_exp_multisynapse.h
+ voltmeter = nest.Create('voltmeter', 1, {'withgid': True})
+
+ delays=[1.0, 300.0]
+ w=[1.0, 1.0, 1.0, 1.0]

This comment has been minimized.

@heplesser

heplesser Dec 7, 2016

Contributor
  • Insert spaces around =.
  • Why four weights, and why the very long delay? I think 30. would be enough.
  • I would also make the second weight different from the first.
@heplesser

heplesser Dec 7, 2016

Contributor
  • Insert spaces around =.
  • Why four weights, and why the very long delay? I think 30. would be enough.
  • I would also make the second weight different from the first.
models/gif_cond_exp_multisynapse.h
+
+ nest.Connect(voltmeter, neuron)
+
+ nest.Simulate(1000.0)

This comment has been minimized.

@heplesser

heplesser Dec 7, 2016

Contributor

If you reduce the delay above, you can reduce simlation time here.

@heplesser

heplesser Dec 7, 2016

Contributor

If you reduce the delay above, you can reduce simlation time here.

models/gif_cond_exp_multisynapse.h
+
+ neuron = nest.Create('gif_cond_exp_multisynapse')
+ nest.SetStatus(neuron, {'E_rev':[0.0, -85.0],
+ 'tau_syn':[4.0, 8.0]})

This comment has been minimized.

@heplesser

heplesser Dec 7, 2016

Contributor

You can send the parameter dictionary as params parameter to the Create() call, no need to call SetStatus() separately.

@heplesser

heplesser Dec 7, 2016

Contributor

You can send the parameter dictionary as params parameter to the Create() call, no need to call SetStatus() separately.

models/gif_cond_exp_multisynapse.h
+ Example:
+
+ import nest
+ import numpy as np

This comment has been minimized.

@heplesser

heplesser Dec 7, 2016

Contributor

I would drop the imports here, the example should be just about this model.

@heplesser

heplesser Dec 7, 2016

Contributor

I would drop the imports here, the example should be just about this model.

models/gif_cond_exp_multisynapse.h
+ spike = nest.Create('spike_generator', params = {'spike_times':
+ np.array([10.0])})
+
+ voltmeter = nest.Create('voltmeter', 1, {'withgid': True})

This comment has been minimized.

@heplesser

heplesser Dec 7, 2016

Contributor

I think I would leave the voltmeter out as well, it does not really tell us much about the model. You can also drop the plotting section towards the end of the example. Only up to the Connect() call is really interesting.

@heplesser

heplesser Dec 7, 2016

Contributor

I think I would leave the voltmeter out as well, it does not really tell us much about the model. You can also drop the plotting section towards the end of the example. Only up to the Connect() call is really interesting.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 7, 2016

Contributor

👍 from me.

Contributor

heplesser commented Dec 7, 2016

👍 from me.

@hesam-setareh

This comment has been minimized.

Show comment
Hide comment
@hesam-setareh

hesam-setareh Dec 8, 2016

Contributor

I've gone through the code and it is fine. The only thing I do not get is that why the neuron should always have at least one synapse (tau_syn_.size() == 0 causes error). For example if we only have a neuron connected to a current injector, it does not necessarily need a synapse.

Contributor

hesam-setareh commented Dec 8, 2016

I've gone through the code and it is fine. The only thing I do not get is that why the neuron should always have at least one synapse (tau_syn_.size() == 0 causes error). For example if we only have a neuron connected to a current injector, it does not necessarily need a synapse.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 9, 2016

Contributor

@hesam-setareh When we reviewed aeif*multisynapse models, we thought that it would make life easier for users to be able to have one synapse pre-configured. But there is no good reason to prohibit the user from configuring the neuron without any synapses.

@hakonsbm Could you remove the prohibition against empty tau_syn? I will make the same change for the remaining multisynapse models.

Contributor

heplesser commented Dec 9, 2016

@hesam-setareh When we reviewed aeif*multisynapse models, we thought that it would make life easier for users to be able to have one synapse pre-configured. But there is no good reason to prohibit the user from configuring the neuron without any synapses.

@hakonsbm Could you remove the prohibition against empty tau_syn? I will make the same change for the remaining multisynapse models.

@hakonsbm

This comment has been minimized.

Show comment
Hide comment
@hakonsbm

hakonsbm Dec 9, 2016

Contributor

@hesam-setareh @heplesser The model now accepts an empty tau_syn, i.e. a configuration without any synapses.

Contributor

hakonsbm commented Dec 9, 2016

@hesam-setareh @heplesser The model now accepts an empty tau_syn, i.e. a configuration without any synapses.

@hesam-setareh

This comment has been minimized.

Show comment
Hide comment
@hesam-setareh

hesam-setareh Dec 9, 2016

Contributor

All good for me.

Contributor

hesam-setareh commented Dec 9, 2016

All good for me.

@heplesser heplesser merged commit 17226fb into nest:master Dec 9, 2016

1 check passed

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

@hakonsbm hakonsbm deleted the hakonsbm:correct_gif_cond_exp branch Dec 9, 2016

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