Skip to content
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

Fixes for the gif multisynapse and quantal_stp model #846

Merged
merged 5 commits into from Mar 9, 2018

Conversation

Projects
None yet
5 participants
@drodarie
Copy link
Contributor

drodarie commented Oct 25, 2017

This request fixes some problems in the gif_cond_exp_multisynapse model relating to the resizing of the neuron state and a minor bug in the quantal_stp model (using thread id instead of virtual process id).

Marc-Oliver Gewaltig
Dimitri Rodarie
Till Schumann

drodarie added some commits Oct 25, 2017

Apply multiple correction to gif_cond_exp_multisynapse model
- Remove unused parameter size_neuron_state
- Change confusing lambda_0 units - now matching Nest units (1/ms)
- Fix state vector size initialization.
- Move state initializations from calibrate to State_::set
- Fix buffers initialization
Fix quantal_stp_connection
get_rng should use thread id instead of virtual process id
@mschmidt87
Copy link

mschmidt87 left a comment

I asked for a short explanation of the changes at one point.

Generally, I think that the language in the documentation of this model should be improved. There are a number of linguistic mistakes, such as "Dynamic of each eta_i is described by..." --> "The dynamics of each eta_i is described by..."

Also, currently this PR cannot be merged due to complaints by the syntax checker.

@@ -212,7 +212,7 @@ Quantal_StpConnection< targetidentifierT >::send( Event& e,
// Compute number of sites that recovered during the interval.
for ( int depleted = n_ - a_; depleted > 0; --depleted )
{
if ( kernel().rng_manager.get_rng( vp )->drand() < ( 1.0 - p_decay ) )
if ( kernel().rng_manager.get_rng( t )->drand() < ( 1.0 - p_decay ) )
{

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 28, 2017

You can now remove line 200 of this file where the virtual process id is retrieved since it is now longer needed.

tau_stc vector of double - Time constants of stc variables in ms.
q_sfa vector of double - Values added to spike-frequency adaptation
(sfa) after each spike emission in mV.
tau_sfa vector of double - Time constants of sfa variables in ms.
Delta_V double - Stochasticity level in mV.
lambda_0 double - Stochastic intensity at firing threshold V_T in 1/s.
V_T_star double - Base threshold in mV
lambda_0 double - Stochastic intensity at firing threshold V_T in 1/ms.

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 28, 2017

In NEST, spike rates are generally defined in spikes/s. Unless I misunderstood the neuron model, lambda_0 is a spike rate. I therefore find it more logical to keep it in 1/s.

This comment has been minimized.

@drodarie

drodarie Nov 28, 2017

Author Contributor

Ok, I will bring lambda's units back to 1/s.

@@ -521,15 +523,6 @@ nest::gif_cond_exp_multisynapse::init_buffers_()
void
nest::gif_cond_exp_multisynapse::calibrate()
{

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 28, 2017

Could you explain your changes at this point in more detail? Do I understand correctly that you can remove this here because the state vectors and buffers are now correctly resized in set and init_buffers_?

This comment has been minimized.

@drodarie

drodarie Nov 28, 2017

Author Contributor

I don't understand what you want me to remove. Is it in the calibrate function ?
In my commit, I fixed the initialization of the state vectors and move them to the correct functions.
I have also fixed the spike buffers initialization which was not adapted to multiple synapses.

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 28, 2017

No need to remove anything, I just wanted some more explanation. It is fine for me now.

@@ -143,8 +143,7 @@ nest::gif_cond_exp_multisynapse::Parameters_::Parameters_()
}

nest::gif_cond_exp_multisynapse::State_::State_( const Parameters_& p )
: y_( STATE_VEC_SIZE, 0.0 )
, size_neuron_state_( 0 )
: y_( STATE_VEC_SIZE + NUMBER_OF_STATES_ELEMENTS_PER_RECEPTOR, 0.0 )

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 28, 2017

Minor comment: I would shorten this variable name to 'NUM_STATE_ELEMENTS_PER_RECEPTOR' . If you agree, then this should also be changed in 'aeif_cond_alpha_multisynapse' for consistency.

This comment has been minimized.

@drodarie

drodarie Nov 28, 2017

Author Contributor

Ok to change the variable to NUM_STATE_ELEMENTS_PER_RECEPTOR.
It is not the Adaptive Exponential Integrate and Fire model (aeif) but rather the Generalized Leeky Integrate and Fire model (glif or gif)

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 28, 2017

Thanks. I am aware which neuron model you have worked on in this PR. What I meant was: If you change the name of this variable to my proposal in this neuron model (gif_cond_exp_multisynapse) (which you did), then we should also change the name accordingly in the other multisynapse neuron models, which are aeif_cond_alpha_multisynapse and aeif_cond_beta_multisynapse, if I am not mistaken (I simply did grep NUMBER_OF_ in the models directory). In aeif_cond_beta_multisynapse I am referring to the variable NUMBER_OF_FIXED_STATES_ELEMENTS, which could be changed to NUM_FIXED_STATE_ELEMENTS.

This could also be done in a separate PR, but since the changes are very small, I would opt for simply adding it this PR, though in a separate commit.

This comment has been minimized.

@drodarie

drodarie Nov 29, 2017

Author Contributor

Sorry, I misunderstood your first comment. I will do the change in the other models.

@mschmidt87
Copy link

mschmidt87 left a comment

Thank you for your changes.

Regarding the issue of lambda_0. Thanks for changing its unit to 1/s. Now there are two locations in the code where you then convert it to 1/ms such that the neuron internally stores it in 1/ms.
I would rather store the value in 1/s and then convert it to ms at the point where you actually use it for in the neuron update (l. 637 in gif_cond_exp_multisynapse.cpp)

@drodarie

This comment has been minimized.

Copy link
Contributor Author

drodarie commented Nov 29, 2017

I understand your point, the code is hence more understandable but then you have to convert lambda_0 at each update (except refractory period).
Should we prefer clarity over optimization ?

@mschmidt87

This comment has been minimized.

Copy link

mschmidt87 commented Nov 30, 2017

No, you are right, it is better to do this conversion only when the number is set or retrieved. Other models do it this way, too, so the current solution is better.

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018

@stinebuu

This comment has been minimized.

Copy link
Contributor

stinebuu commented Mar 7, 2018

@drodarie could you please run extras/check_code_style.sh and fix the problems? This will make travis happy.

@heplesser
Copy link
Contributor

heplesser left a comment

@drodarie The default value for lambda_0_ needs to be in 1/ms, and some comments need to be adjusted, see inline comments. The general approach in NEST is that the user specifies rates in 1/s, and these get converted to 1/ms by the get()/set() methods, so that they can directly be used with NEST's time in ms during update(). This is the most efficient, as you pointed out in the discussion earlier. The incorrect value in the code now may explain why the test for the model fails.

There are also some code formatting issues. You can get the details by running extras/check_code_style.sh in the source directory and fix most issues using clang-format-3.6. You need to add curly braces around single-line blocks manually, though.

@@ -127,11 +127,11 @@ nest::gif_cond_exp_multisynapse::Parameters_::Parameters_()
, V_reset_( -55.0 ) // mV
, Delta_V_( 0.5 ) // mV
, V_T_star_( -35 ) // mV
, lambda_0_( 0.001 ) // 1/ms
, lambda_0_( 1. ) // 1/s

This comment has been minimized.

@heplesser

heplesser Mar 8, 2018

Contributor

This must remain in milliseconds, since the value is used as

const double lambda = P_.lambda_0_ * std::exp( ( S_.V_ - S_.sfa_ ) / P_.Delta_V_ );
...
-numerics::expm1( -lambda * Time::get_resolution().get_ms() )

i.e., it is multiplied by a value in milliseconds.

The user provides/retrieves lambda_0 in 1/s; the value is converted to the internal units of 1/ms in Parameters_::get()/set(). This is the standard approach in NEST.

@@ -240,7 +240,7 @@ class gif_cond_exp_multisynapse : public Archiving_Node
double V_reset_;
double Delta_V_;
double V_T_star_;
double lambda_0_; /** 1/ms */
double lambda_0_; /** 1/s */

This comment has been minimized.

@heplesser

heplesser Mar 8, 2018

Contributor

Must remain 1/ms, see above.

Fix lambda_0 units.
Fix code format.
@heplesser

This comment has been minimized.

Copy link
Contributor

heplesser commented Mar 9, 2018

@drodarie Thank you!

@heplesser heplesser merged commit cdc2ca8 into nest:master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.