Added neuron model aeif_cond_beta_multisynapse using GSL ODE solver (see issue 438) #439

Merged
merged 33 commits into from Sep 27, 2016

Conversation

Projects
None yet
4 participants
@golosio
Contributor

golosio commented Aug 1, 2016

Multisynaptic neuron model with synaptic conductance modeled by the sum of two exponential functions, with independent synaptic rise times and decay times, as described in A. Roth and M.C.W. van Rossum in Computational Modeling Methods for Neuroscientists, MIT Press 2013, Chapter 6. The model is an extension of the conductance based exponential integrate-and-fire neuron model according to Brette and Gerstner (2005), as implemented in aeif_cond_alpha_multisynapse
This pull request deal with the issue #438

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 3, 2016

Contributor

@golosio Thank you for your contribution! We are currently trying to properly understand and handle some numerical issues with aeif models in general (see #229). I would prefer to solve them in the simple case of the aeif_cond_alpha model first, before adding new aeif models. In that way, we can ensure that all new models will use the best numerics. I hope that this is okay for you.

Another pre-requisite for merging your model into the NEST master repository is a signed Contributor License Agreement, which you can download from our website and send me by mail.

Contributor

heplesser commented Aug 3, 2016

@golosio Thank you for your contribution! We are currently trying to properly understand and handle some numerical issues with aeif models in general (see #229). I would prefer to solve them in the simple case of the aeif_cond_alpha model first, before adding new aeif models. In that way, we can ensure that all new models will use the best numerics. I hope that this is okay for you.

Another pre-requisite for merging your model into the NEST master repository is a signed Contributor License Agreement, which you can download from our website and send me by mail.

@golosio golosio changed the title from Added neuron model aeif_cond_2exp_multisynapse (see issue 438) to Added neuron models aeif_cond_2exp_multisynapse and aeif_cond_beta_multisynapse (see issue 438) Aug 4, 2016

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 4, 2016

Contributor

@heplesser Sure, that's fine. Meanwhile I added the (equivalent) model aeif_cond_beta_multisynapse. I compared the two and they give the same results, which is not surprising as the underlying differential equations have the same solutions. aeif_cond_beta_multisynapse is closer to the original model aeif_cond_alpha_multisynapse, and it is based on the same differential equations as those proposed by dnao in "Create iaf_cond_beta" #405 . so it should be a quicker review, when you feel you can do it. By the way, if you think it can be useful I can help with the revision of iaf_cond_beta. Let me know...

Contributor

golosio commented Aug 4, 2016

@heplesser Sure, that's fine. Meanwhile I added the (equivalent) model aeif_cond_beta_multisynapse. I compared the two and they give the same results, which is not surprising as the underlying differential equations have the same solutions. aeif_cond_beta_multisynapse is closer to the original model aeif_cond_alpha_multisynapse, and it is based on the same differential equations as those proposed by dnao in "Create iaf_cond_beta" #405 . so it should be a quicker review, when you feel you can do it. By the way, if you think it can be useful I can help with the revision of iaf_cond_beta. Let me know...

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 4, 2016

Contributor

@golosio I am a bit confused: isn't the beta function precisely the difference-of-two-exponentials? So what precisely is the difference between the _beta_ and the _2exp_ model?

Btw, our preferred integrator for AEIF neurons is at present the GSL ODE solver used in aeif_cond_alpha. Would you consider to convert your code to that solver? In #229, we are currently looking at even better solvers that handle threshold detection as part of the integration.

Concerning contributions to #405, I'd suggest that you contact @dnao directly, since it is his/her code.

Contributor

heplesser commented Aug 4, 2016

@golosio I am a bit confused: isn't the beta function precisely the difference-of-two-exponentials? So what precisely is the difference between the _beta_ and the _2exp_ model?

Btw, our preferred integrator for AEIF neurons is at present the GSL ODE solver used in aeif_cond_alpha. Would you consider to convert your code to that solver? In #229, we are currently looking at even better solvers that handle threshold detection as part of the integration.

Concerning contributions to #405, I'd suggest that you contact @dnao directly, since it is his/her code.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 4, 2016

Contributor

Right, the beta function is the difference of two exponential. The two models differ because they are based on a slightly different (but equivalent) system of differential equations, which have the same solution (i.e. the beta function) and it is probably superfluous to have both, but it was useful for me as a check of consistency. Sure, I'll have a look at the GSL ODE solver and see how difficult it would be to adapt my code for it.

Contributor

golosio commented Aug 4, 2016

Right, the beta function is the difference of two exponential. The two models differ because they are based on a slightly different (but equivalent) system of differential equations, which have the same solution (i.e. the beta function) and it is probably superfluous to have both, but it was useful for me as a check of consistency. Sure, I'll have a look at the GSL ODE solver and see how difficult it would be to adapt my code for it.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 4, 2016

Contributor

@golosio Good. While cross-validation of models is very useful, we want to keep the number of models in the NEST distribution within sensible limits. So I would suggest that you remove the _2exp_ variant.

Contributor

heplesser commented Aug 4, 2016

@golosio Good. While cross-validation of models is very useful, we want to keep the number of models in the NEST distribution within sensible limits. So I would suggest that you remove the _2exp_ variant.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 4, 2016

Contributor

Sure, that's easy!

Contributor

golosio commented Aug 4, 2016

Sure, that's easy!

@golosio golosio changed the title from Added neuron models aeif_cond_2exp_multisynapse and aeif_cond_beta_multisynapse (see issue 438) to Added neuron model aeif_cond_beta_multisynapse (see issue 438) Aug 4, 2016

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 4, 2016

Contributor

@heplesser I removed the 2exp variant

Contributor

golosio commented Aug 4, 2016

@heplesser I removed the 2exp variant

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 4, 2016

Contributor

@heplesser ok, I'm working on the gsl ode version, it should not be too much work if I use aeif_cond_alpha as a base.

Contributor

golosio commented Aug 4, 2016

@heplesser ok, I'm working on the gsl ode version, it should not be too much work if I use aeif_cond_alpha as a base.

@golosio golosio changed the title from Added neuron model aeif_cond_beta_multisynapse (see issue 438) to Added neuron model aeif_cond_beta_multisynapse using GSL ODE solver (see issue 438) Aug 6, 2016

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 6, 2016

Contributor

@heplesser ok, I adapted my code for using the GSL ODE solver and tested it. You can find the new version in the updated pull request.

Contributor

golosio commented Aug 6, 2016

@heplesser ok, I adapted my code for using the GSL ODE solver and tested it. You can find the new version in the updated pull request.

models/aeif_cond_beta_multisynapse.cpp
+ , V_th( -50.4 ) // mV
+ , I_e( 0.0 ) // pA
+ // , MAXERR( 1.0e-10 ) // mV
+ // , HMIN( 1.0e-3 ) // ms

This comment has been minimized.

@heplesser

heplesser Aug 8, 2016

Contributor

Please remove commented-out code.

@heplesser

heplesser Aug 8, 2016

Contributor

Please remove commented-out code.

+multisynapse_neuron << /g_L g_L >> SetStatus
+multisynapse_neuron << /E_in E_in >> SetStatus
+
+multisynapse_neuron ShowStatus

This comment has been minimized.

@heplesser

heplesser Aug 8, 2016

Contributor

Useful for development, but I suggest to remove it since it will not be of use in the actual test.

@heplesser

heplesser Aug 8, 2016

Contributor

Useful for development, but I suggest to remove it since it will not be of use in the actual test.

+ /W exch def
+ /delay exch def
+ % connect spike generator to each port
+ [sg] [multisynapse_neuron] /one_to_one << /model /static_synapse /delay delay /weight W /receptor_type syn_id >> Connect

This comment has been minimized.

@heplesser

heplesser Aug 8, 2016

Contributor

Break the line into several shorter lines.

@heplesser

heplesser Aug 8, 2016

Contributor

Break the line into several shorter lines.

+ between the membrane potential and the rest potential V(t) will be
+ approximately proportional to the synaptic conductance g(t), which can
+ be computed analitically. At the end, the script compares the simulated
+ values of V(t) with the approximate theoretical prediction.

This comment has been minimized.

@heplesser

heplesser Aug 8, 2016

Contributor

Could you add a derivation (or reference to a published derivation) of the approximation you use as reference?

@heplesser

heplesser Aug 8, 2016

Contributor

Could you add a derivation (or reference to a published derivation) of the approximation you use as reference?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 8, 2016

Contributor

@golosio This looks very fine now, just a few details left, see my inline comments. I will soon suggest a second reviewer.

Contributor

heplesser commented Aug 8, 2016

@golosio This looks very fine now, just a few details left, see my inline comments. I will soon suggest a second reviewer.

golosio added some commits Aug 9, 2016

Added derivation of the approximation used as reference in
test_aeif_cond_beta_multisynapse.sli
Fixes to formatting
Removed commented-out code from aeif_cond_beta_multisynapse.cpp
Included in aeif_cond_beta_multisynapse the possibility of the limiti…
…ng case

when synaptic rise time is equal to the decay time (which corresponds
to the alpha function)
@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 9, 2016

Contributor

@heplesser I made all the changes that you indicated. As you suggested, I added a shortened version of the derivation as a comment to the sli test. Furthermore I included the possibility of the limiting case when the synaptic rise time is equal to the decay time, which corresponds to the alpha function. This can be useful because it gives the possibility to have, in the same neuron, both types of models on different synapses. Just to let you know, I made an additional check of consistency, setting the decay time very close to the rise time, and compared the V(t) curve with that obtained using the aeif_cond_alpha_multisynapse model, and they superimpose, as expected.

Contributor

golosio commented Aug 9, 2016

@heplesser I made all the changes that you indicated. As you suggested, I added a shortened version of the derivation as a comment to the sli test. Furthermore I included the possibility of the limiting case when the synaptic rise time is equal to the decay time, which corresponds to the alpha function. This can be useful because it gives the possibility to have, in the same neuron, both types of models on different synapses. Just to let you know, I made an additional check of consistency, setting the decay time very close to the rise time, and compared the V(t) curve with that obtained using the aeif_cond_alpha_multisynapse model, and they superimpose, as expected.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 9, 2016

Contributor

@heplesser There seem to be a problem with mpi
Running test 'selftests/test_pass.sli'... *** Error in `mpirun': munmap_chunk(): invalid pointer
modprobe: ERROR: could not insert 'fglrx': No such device
I don't think this can depend on the changes that I made...

Contributor

golosio commented Aug 9, 2016

@heplesser There seem to be a problem with mpi
Running test 'selftests/test_pass.sli'... *** Error in `mpirun': munmap_chunk(): invalid pointer
modprobe: ERROR: could not insert 'fglrx': No such device
I don't think this can depend on the changes that I made...

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 10, 2016

Contributor

@golosio this crash is most likely completely unrelated to your changes. fglrx is the graphics driver for AMD graphics cards and I have no clue why a reload of this was triggered. I've restarted the jobs.

Contributor

jougs commented Aug 10, 2016

@golosio this crash is most likely completely unrelated to your changes. fglrx is the graphics driver for AMD graphics cards and I have no clue why a reload of this was triggered. I've restarted the jobs.

Fixes in aeif_cond_beta_multisynapse to handle the case when rise tim…
…e and decay

time are different but very close to each other, to avoid that some
denominators were zero.
g0_ex_ and g0_in_ replaced by a single vector g0_, as they are always equal to
each other.
Other smaller fixes to the code
@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 11, 2016

Contributor

@heplesser I made all the changes that you indicated, except for two:

  1. decay time < rise time : for other functions it could be perfectly acceptable, but for the beta functions it would change the sign of synaptic conductance, as exp(-t/tau_rise) would be greater than exp(-t/tau_decay). Although it is not a problem from a mathematical point of view, it could have unexpected consequences and create confusion, I would avoid it.
  2. Only one spike generator needed: it is true, but there is an issue. If I try to use only one generator I get the error: pynestkernel.NESTError: IllegalConnection in Connect_g_g_D_D: Creation of connection is not possible because:
    All outgoing connections from a device must use the same synapse type.
    This issue was already raised by other users, see
    NeuralEnsemble/PyNN#352
    it's in PyNN but it is related to nest. Indeed, the example of aeif_cond_alpha_multisynapse.h does not work for this reason. Maybe we should open an issue?
Contributor

golosio commented Aug 11, 2016

@heplesser I made all the changes that you indicated, except for two:

  1. decay time < rise time : for other functions it could be perfectly acceptable, but for the beta functions it would change the sign of synaptic conductance, as exp(-t/tau_rise) would be greater than exp(-t/tau_decay). Although it is not a problem from a mathematical point of view, it could have unexpected consequences and create confusion, I would avoid it.
  2. Only one spike generator needed: it is true, but there is an issue. If I try to use only one generator I get the error: pynestkernel.NESTError: IllegalConnection in Connect_g_g_D_D: Creation of connection is not possible because:
    All outgoing connections from a device must use the same synapse type.
    This issue was already raised by other users, see
    NeuralEnsemble/PyNN#352
    it's in PyNN but it is related to nest. Indeed, the example of aeif_cond_alpha_multisynapse.h does not work for this reason. Maybe we should open an issue?
models/aeif_cond_beta_multisynapse.cpp
- }
- else
+ // denominator is computed here to check that it is != 0
+ double denom1 = P_.taus_decay[ i ] - P_.taus_rise[ i ];

This comment has been minimized.

@heplesser

heplesser Aug 11, 2016

Contributor

Your code is correct, but the logic a bit difficult to follow, especially the if ( denom2 == 0 ) at the end. Since this code is in calibrate(), thus rarely executed, I would give legibility precedence over performance and use

const double denom1 = ...;
const double denom2 = ...;
if ( denom1 == 0 or denom2 == 0 )
{
  // alpha case
}
else
{ 
  // beta case
}
@heplesser

heplesser Aug 11, 2016

Contributor

Your code is correct, but the logic a bit difficult to follow, especially the if ( denom2 == 0 ) at the end. Since this code is in calibrate(), thus rarely executed, I would give legibility precedence over performance and use

const double denom1 = ...;
const double denom2 = ...;
if ( denom1 == 0 or denom2 == 0 )
{
  // alpha case
}
else
{ 
  // beta case
}

This comment has been minimized.

@golosio

golosio Aug 11, 2016

Contributor

@heplesser I agree, indeed first I tried to write the code as you suggest, but the problem is that denom2 depends on t_peak, and before computing t_peak I must check that denom1 is !=0...
I could not find a more readable solution...

@golosio

golosio Aug 11, 2016

Contributor

@heplesser I agree, indeed first I tried to write the code as you suggest, but the problem is that denom2 depends on t_peak, and before computing t_peak I must check that denom1 is !=0...
I could not find a more readable solution...

This comment has been minimized.

@heplesser

heplesser Aug 11, 2016

Contributor

@golosio I should think before I comment ... But how about the following:

const double denom1 = ...;
double denom2 = 0;
if ( denom1 != 0 )
{
  const double t_p = ...;
  denom2 = ...;
}
if ( denom2 == 0 )  // if denom1 == 0, denom2 == 0 is implied, no need to test denom1 here again
{
  // alpha
}
else
{
  // beta
}

This avoids have an if ( denom2 !=0 ) in one place and an if ( denom2 == 0 ) a few lines further down.

@heplesser

heplesser Aug 11, 2016

Contributor

@golosio I should think before I comment ... But how about the following:

const double denom1 = ...;
double denom2 = 0;
if ( denom1 != 0 )
{
  const double t_p = ...;
  denom2 = ...;
}
if ( denom2 == 0 )  // if denom1 == 0, denom2 == 0 is implied, no need to test denom1 here again
{
  // alpha
}
else
{
  // beta
}

This avoids have an if ( denom2 !=0 ) in one place and an if ( denom2 == 0 ) a few lines further down.

This comment has been minimized.

@golosio

golosio Aug 11, 2016

Contributor

yes, this looks fine!

@golosio

golosio Aug 11, 2016

Contributor

yes, this looks fine!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 11, 2016

Contributor

@golosio If you include the prefactor (Roth & van Rossum, Eqs 6.4-6.6), the beta function is fully symmetric under exchange of rise and decay times. This means two things: (i) The function will always be positive, no matter which of the two times is larger. (ii) The shorter of the two times will always be the rise time, no matter what it is called. Thus, requiring tau_rise <= tau_decay makes sense, because otherwise one will just get confusing behavior.

I overlooked the problem with the restriction to a single synapse type (even though I implemented it myself, see trac ticket 737). The restriction is in place to ensure that a callback mechanism used by some stimulating devices works correctly.

But you don't need the different synapse models at all, you can just pass the parameters directly in the connect call:

nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 1.0,
                                      'receptor_type': 1})
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 100.0,
                                      'receptor_type': 2})
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 300.0,
                                      'receptor_type': 3})
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 500.0,
                                      'receptor_type': 4})

or even more compact (with slightly different delay values):

for syn in range(4):
    nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                          'weight': 1.0,
                                          'delay': 1.0 + syn * 200.,
                                          'receptor_type': 1 + syn})
Contributor

heplesser commented Aug 11, 2016

@golosio If you include the prefactor (Roth & van Rossum, Eqs 6.4-6.6), the beta function is fully symmetric under exchange of rise and decay times. This means two things: (i) The function will always be positive, no matter which of the two times is larger. (ii) The shorter of the two times will always be the rise time, no matter what it is called. Thus, requiring tau_rise <= tau_decay makes sense, because otherwise one will just get confusing behavior.

I overlooked the problem with the restriction to a single synapse type (even though I implemented it myself, see trac ticket 737). The restriction is in place to ensure that a callback mechanism used by some stimulating devices works correctly.

But you don't need the different synapse models at all, you can just pass the parameters directly in the connect call:

nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 1.0,
                                      'receptor_type': 1})
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 100.0,
                                      'receptor_type': 2})
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 300.0,
                                      'receptor_type': 3})
nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                      'weight': 1.0,
                                      'delay': 500.0,
                                      'receptor_type': 4})

or even more compact (with slightly different delay values):

for syn in range(4):
    nest.Connect(spike, neuron, syn_spec={'model': 'static_synapse',
                                          'weight': 1.0,
                                          'delay': 1.0 + syn * 200.,
                                          'receptor_type': 1 + syn})
@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 11, 2016

Contributor

@heplesser Done!

Contributor

golosio commented Aug 11, 2016

@heplesser Done!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 11, 2016

Contributor

@golosio 👍 from me, and thanks for the effort! Before merging, we should still wait until the test system works again for all configurations.

@jougs Any comments?

Contributor

heplesser commented Aug 11, 2016

@golosio 👍 from me, and thanks for the effort! Before merging, we should still wait until the test system works again for all configurations.

@jougs Any comments?

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Aug 11, 2016

Contributor

@heplesser thank you for your constructive review!

Contributor

golosio commented Aug 11, 2016

@heplesser thank you for your constructive review!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 25, 2016

Contributor

@jougs Do you have any objections to merging this PR now that all tests have passed?

Contributor

heplesser commented Aug 25, 2016

@jougs Do you have any objections to merging this PR now that all tests have passed?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 29, 2016

Contributor

@heplesser: I was just commenting here because of the failing builds, not as a reviewer.

Maybe @ingablundell or @DimitriPlotnikov could have a look?

Contributor

jougs commented Aug 29, 2016

@heplesser: I was just commenting here because of the failing builds, not as a reviewer.

Maybe @ingablundell or @DimitriPlotnikov could have a look?

@DimitriPlotnikov

This comment has been minimized.

Show comment
Hide comment
@DimitriPlotnikov

DimitriPlotnikov Sep 9, 2016

@heplesser @jougs my first impression is that the model is implemented well. Before I proceed with more detailed review, I would have an acknowledgment that the model is important enough to be included into NEST repository.

@heplesser @jougs my first impression is that the model is implemented well. Before I proceed with more detailed review, I would have an acknowledgment that the model is important enough to be included into NEST repository.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 9, 2016

Contributor

Thank you @DimitriPlotnikov.

Contributor

golosio commented Sep 9, 2016

Thank you @DimitriPlotnikov.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Sep 10, 2016

Contributor

@DimitriPlotnikov Yes, the model is relevant (otherwise I wouldn't have given it a thumb up).

Contributor

heplesser commented Sep 10, 2016

@DimitriPlotnikov Yes, the model is relevant (otherwise I wouldn't have given it a thumb up).

@DimitriPlotnikov

This comment has been minimized.

Show comment
Hide comment
@DimitriPlotnikov

DimitriPlotnikov Sep 10, 2016

@heplesser Thank you for your replay. I still a bit confused with this 'kitschy' emojies which can appear at arbitary place in the discussion. I'll tackle it next week.

@heplesser Thank you for your replay. I still a bit confused with this 'kitschy' emojies which can appear at arbitary place in the discussion. I'll tackle it next week.

models/aeif_cond_beta_multisynapse.h
+
+ /** buffers and sums up incoming spikes/currents */
+ std::vector< RingBuffer > spike_exc_;
+ std::vector< RingBuffer > spike_inh_;

This comment has been minimized.

@DimitriPlotnikov

DimitriPlotnikov Sep 10, 2016

Here I need qualified help of @heplesser:

We already discussed the issue with our interpretation of inhibitory and excitatory buffers in the current NEST state. In a model with one buffer (iaf_psc_exp) the logic to use the sigh of the weight to route spike events is at least plausible. In the case of the mutlisynapse model it leads to: a) you always have the same number of inh und exc buffers b) you have to buffers (inh/exc) at the same portnumber c) finally, this implementation is inconsistent with the implementation of the iaf_psc_alpha_multisynapse and iaf_psc_exp_multisynapse.

@DimitriPlotnikov

DimitriPlotnikov Sep 10, 2016

Here I need qualified help of @heplesser:

We already discussed the issue with our interpretation of inhibitory and excitatory buffers in the current NEST state. In a model with one buffer (iaf_psc_exp) the logic to use the sigh of the weight to route spike events is at least plausible. In the case of the mutlisynapse model it leads to: a) you always have the same number of inh und exc buffers b) you have to buffers (inh/exc) at the same portnumber c) finally, this implementation is inconsistent with the implementation of the iaf_psc_alpha_multisynapse and iaf_psc_exp_multisynapse.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 11, 2016

Contributor

@DimitriPlotnikov I took as reference aeif_cond_alpha_multisynapse, but I agree with your observations. If you already discussed this issue for the iaf_psc_*_multisynapse models, probably there is no reason to keep two separate buffers for inhibitory and excitatory connections. We have to be careful that in conductance based models the driving force is different for inhibitory vs excitatory signals, but this should not be a big deal. I will try to modify the code and let you know.

Contributor

golosio commented Sep 11, 2016

@DimitriPlotnikov I took as reference aeif_cond_alpha_multisynapse, but I agree with your observations. If you already discussed this issue for the iaf_psc_*_multisynapse models, probably there is no reason to keep two separate buffers for inhibitory and excitatory connections. We have to be careful that in conductance based models the driving force is different for inhibitory vs excitatory signals, but this should not be a big deal. I will try to modify the code and let you know.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 11, 2016

Contributor

@DimitriPlotnikov I modified the code according to your comment. Now there is only one buffer for both types of events, inhibitory or excitatory.
I compared the results of the test, which uses both exc and inh connections, with the old and with the updated code, and they are exactly the same.
Probably this modification should be done to all conductance-based multisynapse and multicompartment models.

Contributor

golosio commented Sep 11, 2016

@DimitriPlotnikov I modified the code according to your comment. Now there is only one buffer for both types of events, inhibitory or excitatory.
I compared the results of the test, which uses both exc and inh connections, with the old and with the updated code, and they are exactly the same.
Probably this modification should be done to all conductance-based multisynapse and multicompartment models.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Sep 12, 2016

Contributor

@golosio @DimitriPlotnikov The issue of the exc/inh buffer also occurs in #261 for the gif_cond_exp_multisynapse models. I put it on the agenda for the NEST developer VC on 19 Sept.

Contributor

heplesser commented Sep 12, 2016

@golosio @DimitriPlotnikov The issue of the exc/inh buffer also occurs in #261 for the gif_cond_exp_multisynapse models. I put it on the agenda for the NEST developer VC on 19 Sept.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 13, 2016

Contributor

@heplesser @DimitriPlotnikov maybe we should seek a more general solution (see the issue that i just opened #477 ). My opinion is that I would not ask Hesam to review his code once more for this issue before merging it, after all it only affects performances, it can be fixed at a later stage.

Contributor

golosio commented Sep 13, 2016

@heplesser @DimitriPlotnikov maybe we should seek a more general solution (see the issue that i just opened #477 ). My opinion is that I would not ask Hesam to review his code once more for this issue before merging it, after all it only affects performances, it can be fixed at a later stage.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 22, 2016

Contributor

@DimitriPlotnikov Did you have a chance to check the code with the changes that I made to meet your observation? Do you have other amendments to propose?

Contributor

golosio commented Sep 22, 2016

@DimitriPlotnikov Did you have a chance to check the code with the changes that I made to meet your observation? Do you have other amendments to propose?

@DimitriPlotnikov

This comment has been minimized.

Show comment
Hide comment
@DimitriPlotnikov

DimitriPlotnikov Sep 22, 2016

I'll do it tomorrow.

I'll do it tomorrow.

@DimitriPlotnikov

From the implementation perspective the model is well implemented. I also wrote an "brunel" simulation based on the provided model and it produced some results (Excitatory rate : 4.40 Hz, Inhibitory rate : 10.20 Hz)
e.g. non zero rates. After fixing mentioned 2 minor issues, it can be finally approved.

models/aeif_cond_beta_multisynapse.cpp
+ // denominator is computed here to check that it is != 0
+ double denom1 = P_.taus_decay[ i ] - P_.taus_rise[ i ];
+ double denom2 = 0;
+ if ( denom1 != 0 )

This comment has been minimized.

@DimitriPlotnikov

DimitriPlotnikov Sep 23, 2016

Could you please add an informative comment why are you doing this computation and checks? Can it be already detected in the Parameters_::set?

@DimitriPlotnikov

DimitriPlotnikov Sep 23, 2016

Could you please add an informative comment why are you doing this computation and checks? Can it be already detected in the Parameters_::set?

+ Adapted by Bruno Golosio from aeif_cond_alpha_multisynapse
+ SeeAlso: aeif_cond_alpha_multisynapse
+ */
+

This comment has been minimized.

@DimitriPlotnikov

DimitriPlotnikov Sep 23, 2016

I tried to run the example that you provide in the commet. I would add

import nest 
import numpy as np

to make it an executable python program.

Another issue is that the resulting script fails at my machine with the following stacktrace:


Traceback (most recent call last):
  File "aeif_cond_alpha_multisynapse.py", line 24, in <module>
    nest.Connect(spike, neuron, syn_spec="synapse2")
  File "/home/nestml/repositories/nest_repo/bld_master_nompi/lib/python2.7/site-packages/nest/lib/hl_api_helper.py", line 230, in stack_checker_func
    return f(*args, **kwargs)
  File "/home/nestml/repositories/nest_repo/bld_master_nompi/lib/python2.7/site-packages/nest/lib/hl_api_connections.py", line 314, in Connect
    sr('Connect')
  File "/home/nestml/repositories/nest_repo/bld_master_nompi/lib/python2.7/site-packages/nest/__init__.py", line 91, in catching_sli_run
    errorname, commandname, message))
pynestkernel.NESTError: IllegalConnection in Connect_g_g_D_D: Creation of connection is not possible because:
All outgoing connections from a device must use the same synapse type.

Am I doing something wrong? Otherwise, could you fix the comment?

@DimitriPlotnikov

DimitriPlotnikov Sep 23, 2016

I tried to run the example that you provide in the commet. I would add

import nest 
import numpy as np

to make it an executable python program.

Another issue is that the resulting script fails at my machine with the following stacktrace:


Traceback (most recent call last):
  File "aeif_cond_alpha_multisynapse.py", line 24, in <module>
    nest.Connect(spike, neuron, syn_spec="synapse2")
  File "/home/nestml/repositories/nest_repo/bld_master_nompi/lib/python2.7/site-packages/nest/lib/hl_api_helper.py", line 230, in stack_checker_func
    return f(*args, **kwargs)
  File "/home/nestml/repositories/nest_repo/bld_master_nompi/lib/python2.7/site-packages/nest/lib/hl_api_connections.py", line 314, in Connect
    sr('Connect')
  File "/home/nestml/repositories/nest_repo/bld_master_nompi/lib/python2.7/site-packages/nest/__init__.py", line 91, in catching_sli_run
    errorname, commandname, message))
pynestkernel.NESTError: IllegalConnection in Connect_g_g_D_D: Creation of connection is not possible because:
All outgoing connections from a device must use the same synapse type.

Am I doing something wrong? Otherwise, could you fix the comment?

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 23, 2016

Contributor

@DimitriPlotnikov Thank you for your review. I made the changes that you requested. I improved the comments and completed the example. I already noticed that the example provided in aeif_cond_alpha_multisynapse don't work, see my comments at the end of #439 (comment) and the following reply from @heplesser . On the other hand, I fixed my example and it should work.
It's interesting that you used the model in a Brunel simulation. I'm also working on a kind of balanced network, I'm trying to implement with a more realistic neuron model the network described in Lim S, Goldman M, Nature Neuroscience 16, 1306–1314 (2013) http://www.nature.com/neuro/journal/v16/n9/full/nn.3492.html

Contributor

golosio commented Sep 23, 2016

@DimitriPlotnikov Thank you for your review. I made the changes that you requested. I improved the comments and completed the example. I already noticed that the example provided in aeif_cond_alpha_multisynapse don't work, see my comments at the end of #439 (comment) and the following reply from @heplesser . On the other hand, I fixed my example and it should work.
It's interesting that you used the model in a Brunel simulation. I'm also working on a kind of balanced network, I'm trying to implement with a more realistic neuron model the network described in Lim S, Goldman M, Nature Neuroscience 16, 1306–1314 (2013) http://www.nature.com/neuro/journal/v16/n9/full/nn.3492.html

@DimitriPlotnikov

This comment has been minimized.

Show comment
Hide comment
@DimitriPlotnikov

DimitriPlotnikov Sep 26, 2016

This PR can be merged.

This PR can be merged.

@golosio

This comment has been minimized.

Show comment
Hide comment
@golosio

golosio Sep 26, 2016

Contributor

Thank you @DimitriPlotnikov

Contributor

golosio commented Sep 26, 2016

Thank you @DimitriPlotnikov

@heplesser

@golosio Thank you for your efforts! The code is ready for merge 👍!

@heplesser heplesser merged commit 4b94532 into nest:master Sep 27, 2016

1 check passed

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

@heplesser heplesser referenced this pull request Oct 6, 2016

Merged

Gif models #261

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