Add new neuron model hh_cond_beta_gap_traub #1098

dnao · 2019-01-05T12:57:19Z

I have created a new neuron model by modifying the hh_psc_alpha_gap and hh_cond_exp_traub models. It uses beta-function synapses and gap junction coupling. Though I have attempted to follow the existing models closely, feedback would be appreciated as there may be some interactions between the dynamics and synaptic/gap junction conductance which I have not considered.

janhahne

@dnao Before I take a closer look please run extras/format_all_c_c++_files.sh to fix the formatting issues and add the new model to the ignore list in testsuite/regressiontests/ticket-421.sli and pynest/nest/tests/test_refractory.py to prevent those unit tests from failing.

janhahne · 2019-01-07T15:52:04Z

models/iaf_cond_beta.cpp

@@ -373,7 +373,7 @@ nest::iaf_cond_beta::get_normalisation_factor( double tau_rise,
  double tau_decay )
 {
  // Factor used to normalise the synaptic conductance such that
-  //
+  // incoming spike causes a conductance of amplitude 1 nS.


This seems unrelated to this PR, but its probably fine to fix it here

dnao · 2019-01-08T10:36:00Z

Done, though I haven't had much luck using clang-format with the config file in the nest directory. I find that the CI requires some additional formatting different to what the script produces. I'm using clang 7.0 so perhaps I should switch to 3.6? Would it be a bad idea to squash and rebase all these commits to make this PR less cluttered?

janhahne · 2019-01-08T10:43:22Z

@dnao Thank you! With clang 3.6 the automatic formatting works (at least for me) without any need for further adjustments, so it is probably a good idea to use version 3.6. Regarding your second question: It is not at all a bad idea, it is very welcome (however not mandatory).

heplesser

@dnao Thank you for your contribution. Please download the NEST Contributor Agreement which you can find at the bottom of http://nest.github.io/nest-simulator/, fill it in, scan it and send it to [email protected].

janhahne

@dnao The code (especially the implementation of the gap junctions) looks good to me. I only have two small inline comments and suggest to add a unit test similiar to testsuite/unittests/test_hh_psc_alpha_gap.sli. This would ensure the functionality of the gap-junction implementation in the model in the future (given that it is correct at the moment). You can basicly copy the existing test and just replace the model in lines 73-74 with the new model and adjust the reference solution in lines 111ff with the reference solution of this new model (as stated below)

[[1 -5.999900e+01] [2 -5.998700e+01] [3 -5.996700e+01] [4 -5.994400e+01] [5 -5.991900e+01] [6 -5.989200e+01] [7 -5.986400e+01] [8 -5.983500e+01] [9 -5.980400e+01] [10 -5.977300e+01] [11 -5.974000e+01] [12 -5.970700e+01] [13 -5.967200e+01] [14 -5.963500e+01] [15 -5.959800e+01] [117 -7.791800e+01] [118 -7.778200e+01] [119 -7.764800e+01] [120 -7.751400e+01] [121 -7.738100e+01] [122 -7.724900e+01] [123 -7.711700e+01] [124 -7.698700e+01] [125 -7.685700e+01] [126 -7.672800e+01] [127 -7.660000e+01] [128 -7.647200e+01] [129 -7.634600e+01] [130 -7.622000e+01] [131 -7.609400e+01] [190 -6.982400e+01] [191 -6.973400e+01] [192 -6.964500e+01] [193 -6.955600e+01] [194 -6.946700e+01] [195 -6.937900e+01] [196 -6.929100e+01] [197 -6.920400e+01] [198 -6.911700e+01] [199 -6.903100e+01]]

models/hh_cond_beta_gap_traub.h

models/CMakeLists.txt

dnao · 2019-02-10T22:52:58Z

@janhahne I have added the unit tests you suggested and also fixed the normalisation so incoming spikes cause a peak conductance of 1nS as with other models using beta-function synapses. I will need to fix the same normalisation issue in the iaf_cond_beta model but perhaps it is better if I make a new PR for that?

janhahne · 2019-02-11T13:47:21Z

@dnao Thanks, the test looks fine! I agree that it would be better to create a new PR for the fix of models/iaf_cond_beta. Maybe just undo the (outdated) changes to models/iaf_cond_beta.cpp here and apply the correct normalisation in a new PR.

The indent in models/CMakeLists.txt is still wrong, please have another look there. Also see my inline comment about set_potential and get_potential. Otherwise I am happy with this PR!

@heplesser Did you meant to approve this PR when asking for the NEST Contributor Agreement or do you want to have another look?

dnao · 2019-02-12T04:08:43Z

Thanks @janhahne . I've removed the functions set_potential and get_potential. If these changes are OK now then I will squash them into a single commit.

janhahne

Everything looks fine now, thanks 👍

jougs · 2019-02-21T09:24:36Z

@janhahne, @heplesser: can this be merged now? Thanks!

clinssen

I'm approving this PR based on going through the code, but did not do any functional testing.

clinssen · 2019-02-21T09:35:07Z

models/hh_cond_beta_gap_traub.cpp

+  double denom1 = tau_decay - tau_rise;
+  double denom2 = 0;
+  double normalisation_factor = 0;
+  if ( denom1 != 0 )


I suggest replacing the condition with |denom1| < std::numeric_limits< double >::epsilon()

Also: why is denom2 calculated inside the first if block? Can we use the return-early pattern here?

I agree with the suggestion to check std::abs(denom1) < std::numeric_limits< double > makes sense, also applies to the check for denom2.

Hi, this section of the code is the same as aeif_cond_beta_multisynapse, so I suggest these changes should also apply there.

clinssen · 2019-02-21T09:37:17Z

models/hh_cond_beta_gap_traub.cpp

+      }
+      else
+        // (    threshold    &&     maximum       )
+        if ( S_.y_[ State_::V_M ] >= P_.V_T + 30.


Where does the value 30 come from? Can we give this a name and move it to a private const member variable?

It is exactly the same code as in hh_cond_exp_traub, so I suggest to leave this as it its.

heplesser

@dnao I added suggestions to comments by @clinssen. Once those are fixed, the PR should be ready to merge.

heplesser · 2019-02-25T10:26:28Z

#329 discusses a problem with spike generation in the hh_cond_exp_traub model, which most likely also applies here. So we need to update this model once we have a solution for #329.

heplesser · 2019-03-13T07:44:48Z

@dnao Using Github's web-based tools, I just resolved a merge conflict that had occurred due to merging #1095 into master, see commit bacfb05. Could you take a look at my comments above? If you could respond to them within a few days, we should be able to merge this PR in time for the next NEST release.

heplesser · 2019-03-19T20:46:13Z

@dnao All looks good now, except that one regression test fails. It is expected that this test fails for hh_cond_beta_gap_traub. In order to exclude your model from being tested, please go to file testsuite/regressiontests/ticket-421.sli and add hh_cond_beta_gap_traub to the list of excluded models in the proper alphabetical position:

/exclude_models [/aeif_cond_exp /aeif_cond_alpha /aeif_cond_alpha_RK5 /a2eif_cond_exp /a2eif_cond_exp_HW 
                 /aeif_cond_alpha_multisynapse /aeif_psc_delta_clopath
                 /aeif_psc_exp /aeif_psc_alpha /aeif_psc_delta /aeif_cond_beta_multisynapse /hh_cond_exp_traub
                 /hh_psc_alpha /hh_psc_alpha_clopath /hh_psc_alpha_gap /ht_neuron /ht_neuron_fs
                 /iaf_cond_exp_sfa_rr /izhikevich] def

clang-format for previous changes Added exception to SLI tests for hh_cond_beta_gap_traub

terhorstd requested review from heplesser, janhahne and clinssen January 7, 2019 11:16

janhahne reviewed Jan 7, 2019

View reviewed changes

dnao force-pushed the hh_cond_beta_gap_traub branch from 9acf5cb to 4a392ac Compare January 8, 2019 11:12

heplesser approved these changes Jan 9, 2019

View reviewed changes

janhahne suggested changes Jan 9, 2019

View reviewed changes

models/hh_cond_beta_gap_traub.h Outdated Show resolved Hide resolved

models/CMakeLists.txt Outdated Show resolved Hide resolved

janhahne mentioned this pull request Feb 11, 2019

Remove unused functions in hh_psc_alpha models #1121

Merged

janhahne approved these changes Feb 12, 2019

View reviewed changes

Create hh_cond_beta_gap_traub including unit tests.

a1d2087

dnao force-pushed the hh_cond_beta_gap_traub branch from 40d6b2c to a1d2087 Compare February 12, 2019 20:57

clinssen approved these changes Feb 21, 2019

View reviewed changes

heplesser requested changes Feb 25, 2019

View reviewed changes

Merge branch 'master' into hh_cond_beta_gap_traub

bacfb05

Updated normalisation factor per clinssen and heplesser's suggestions

7ae6998

clang-format for previous changes Added exception to SLI tests for hh_cond_beta_gap_traub

dnao force-pushed the hh_cond_beta_gap_traub branch from 32baa8c to 7ae6998 Compare March 21, 2019 05:28

heplesser approved these changes Mar 21, 2019

View reviewed changes

heplesser changed the title ~~Create hh_cond_beta_gap_traub~~ Add new neuron model hh_cond_beta_gap_traub Mar 21, 2019

heplesser merged commit aab86c5 into nest:master Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new neuron model hh_cond_beta_gap_traub #1098

Add new neuron model hh_cond_beta_gap_traub #1098

dnao commented Jan 5, 2019

janhahne left a comment

janhahne Jan 7, 2019

dnao commented Jan 8, 2019

janhahne commented Jan 8, 2019

heplesser left a comment

janhahne left a comment

dnao commented Feb 10, 2019

janhahne commented Feb 11, 2019

dnao commented Feb 12, 2019

janhahne left a comment

jougs commented Feb 21, 2019

clinssen left a comment

clinssen Feb 21, 2019

heplesser Feb 25, 2019

dnao Mar 18, 2019

clinssen Feb 21, 2019

heplesser Feb 25, 2019

heplesser left a comment

heplesser commented Feb 25, 2019

heplesser commented Mar 13, 2019

heplesser commented Mar 19, 2019

Add new neuron model hh_cond_beta_gap_traub #1098

Add new neuron model hh_cond_beta_gap_traub #1098

Conversation

dnao commented Jan 5, 2019

janhahne left a comment

Choose a reason for hiding this comment

janhahne Jan 7, 2019

Choose a reason for hiding this comment

dnao commented Jan 8, 2019

janhahne commented Jan 8, 2019

heplesser left a comment

Choose a reason for hiding this comment

janhahne left a comment

Choose a reason for hiding this comment

dnao commented Feb 10, 2019

janhahne commented Feb 11, 2019

dnao commented Feb 12, 2019

janhahne left a comment

Choose a reason for hiding this comment

jougs commented Feb 21, 2019

clinssen left a comment

Choose a reason for hiding this comment

clinssen Feb 21, 2019

Choose a reason for hiding this comment

heplesser Feb 25, 2019

Choose a reason for hiding this comment

dnao Mar 18, 2019

Choose a reason for hiding this comment

clinssen Feb 21, 2019

Choose a reason for hiding this comment

heplesser Feb 25, 2019

Choose a reason for hiding this comment

heplesser left a comment

Choose a reason for hiding this comment

heplesser commented Feb 25, 2019

heplesser commented Mar 13, 2019

heplesser commented Mar 19, 2019