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

aEIF psc models #513

Merged
merged 13 commits into from Oct 26, 2016
Merged

aEIF psc models #513

merged 13 commits into from Oct 26, 2016

Conversation

@Silmathoron
Copy link
Member

@Silmathoron Silmathoron commented Oct 6, 2016

This PR adds implementations of the aEIF model with post-synaptic currents:

  • aeif_psc_alpha
  • aeif_psc_exp

Test is provided via an update of test_aeif_lsodar.py and by some of the usual suites such as issue-77.sli, which was modified.

@golosio
Copy link
Contributor

@golosio golosio commented Oct 15, 2016

@Silmathoron @heplesser if you agree I can review this PR

@Silmathoron
Copy link
Member Author

@Silmathoron Silmathoron commented Oct 15, 2016

Of course you can ;)
but I'm waiting for the VC on Monday to (most probably) remove the 2nd dynamics function and solve the conflicts

Copy link
Contributor

@heplesser heplesser left a comment

@Silmathoron Except for a few small things, this looks fine to me.

@@ -94,19 +94,20 @@ nest::aeif_psc_alpha_dynamics( double,
// good compiler will optimize the verbosity away ...

// shorthand for state variables
const double& V = std::min( y[ S::V_M ], node.P_.V_peak_ ); // bound V
const double& V = std::min( y[ S::V_M ], node.P_.V_peak_ ); // bind V to the

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016
Contributor

I would suggest bound V by V_peak

This comment has been minimized.

@Silmathoron

Silmathoron Oct 25, 2016
Author Member

I think this comment relates to an outdated version, current comment is
// bind V to the USER DEFINED V_peak_ value in Parameters.

This comment has been minimized.

@heplesser

heplesser Oct 25, 2016
Contributor

@Silmathoron The English language is a challenging one: "bound" is both past tense of "bind" and a verb in its own right meaning "to confine" or "to limit". The original comment uses "bound" in the second meaning. "Bind" would be incorrect here, since it implied tying the membrane potential to V_peak, instead of just having V_peak as an upper limit. But this discussions shows the need for clearer language, so I would suggest: // enforce upper limit on V_m.

I would not mention "USER DEFINED V_peak_ value in Parameters", since that is clear from the code.

This comment has been minimized.

@Silmathoron

Silmathoron Oct 25, 2016
Author Member

done!

const double I_spike =
node.P_.Delta_T * std::exp( ( V - node.P_.V_th ) / node.P_.Delta_T );
const double I_spike = node.P_.Delta_T == 0. ? 0. : node.P_.g_L
* node.P_.Delta_T * std::exp( ( V - node.P_.V_th ) / node.P_.Delta_T );

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016
Contributor

For clarity, I would put the entire expression after : in parentheses, even though they are not strictly needed in C++.

@@ -94,17 +94,18 @@ nest::aeif_psc_exp_dynamics( double, const double y[], double f[], void* pnode )
// good compiler will optimize the verbosity away ...

// shorthand for state variables
const double& V = std::min( y[ S::V_M ], node.P_.V_peak_ );
const double& V = std::min( y[ S::V_M ], node.P_.V_peak_ ); // bind V to the
// USER DEFINED V_peak_ value in Parameters.

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016
Contributor

See suggestion or alpha above.

const double I_spike =
node.P_.Delta_T * std::exp( ( V - node.P_.V_th ) / node.P_.Delta_T );
const double I_spike = node.P_.Delta_T == 0. ? 0. : node.P_.g_L
* node.P_.Delta_T * std::exp( ( V - node.P_.V_th ) / node.P_.Delta_T );

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016
Contributor

Also here parentheses around all afer : would be helpful.

Silmathoron added 2 commits Oct 25, 2016
@heplesser
Copy link
Contributor

@heplesser heplesser commented Oct 25, 2016

@golosio I am happy with the code, how about you?

@golosio
Copy link
Contributor

@golosio golosio commented Oct 25, 2016

I have just very little suggestions/doubts


updateValue< double >( d, names::gsl_error_tol, gsl_error_tol );

if ( V_reset_ >= V_peak_ )

This comment has been minimized.

@golosio

golosio Oct 25, 2016
Contributor

It seems that also here the check that V_peak_ > V_th is missing

* ---------------------------------------------------------------- */

nest::aeif_psc_alpha::Parameters_::Parameters_()
: V_peak_( 0.0 ) // mV, should not be larger that V_th+10

This comment has been minimized.

@golosio

golosio Oct 25, 2016
Contributor

I've seen this comment in all aeif models, but I do not understand it. Why shouldn't V_peak be larger that V_th+10? Even looking at the default values, the default value of V_th is -50.4, while V_peak is zero, so the difference is larger than 10. Am I missing something?

This comment has been minimized.

@Silmathoron

Silmathoron Oct 25, 2016
Author Member

I never noticed that! It looks completely irrelevant to me...
@heplesser any thought on that?

This comment has been minimized.

@heplesser

heplesser Oct 25, 2016
Contributor

I think the idea here is that the larger the V_th to V_peak is, the worse does the exploding-exponential problem become. But I think we handle this well elsewhere now, so the comment can be removed.


updateValue< double >( d, names::gsl_error_tol, gsl_error_tol );

if ( V_reset_ >= V_peak_ )

This comment has been minimized.

@golosio

golosio Oct 25, 2016
Contributor

See above about the check that V_peak > V_th

This comment has been minimized.

@Silmathoron

Silmathoron Oct 25, 2016
Author Member

Right, I forgot about that, thanks for reminding me! ;)

This comment has been minimized.

@golosio

golosio Oct 25, 2016
Contributor

Besides the very little issues above, everything looks fine to me.

@Silmathoron
Copy link
Member Author

@Silmathoron Silmathoron commented Oct 25, 2016

Ok, I readded the missing test to all aeif_*_alpha models. I guess it went missing during some rearrangement in the dynamics modification.

…hould not be larger that V_th+10" comment on V_peak_
@golosio
Copy link
Contributor

@golosio golosio commented Oct 25, 2016

@Silmathoron @heplesser everythng is ok for me, I think this PR can be merged

@heplesser
Copy link
Contributor

@heplesser heplesser commented Oct 25, 2016

@Silmathoron Thanks for putting the test back in! But I think we should allow V_peak == V_th. Also, I would write the error message as "V_peak >= V_th required.". That is closer to what we use elsewhere and explicitly names the parameters involved.

…all aeif models except beta_multisynapse
@Silmathoron
Copy link
Member Author

@Silmathoron Silmathoron commented Oct 25, 2016

Ok, done! @golosio I'll let you do the changes on the beta_multisynapse model to avoid conflicts

@heplesser
Copy link
Contributor

@heplesser heplesser commented Oct 26, 2016

@Silmathoron Thanks for your efforts, all is well now! Together with @golosio's 👍, I will merge.

@heplesser heplesser merged commit a418160 into nest:master Oct 26, 2016
1 check passed
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.