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

Better documentation of hh_cond_exp_traub and Brette-2007-compatible refractory time #944

Merged
merged 5 commits into from
May 28, 2018

Conversation

heplesser
Copy link
Contributor

@heplesser heplesser commented Apr 27, 2018

When fixing #473 to make t_ref configurable in hh_cond_exp_traub, we configured the model with t_ref==0 as default. This does not work properly, since the spike detection mechanism relies on an absolute refractory peroid. This PR sets the default t_ref=2ms as used in the Brette et al (2007) review and documents the relation of this model to the review more clearly, as well as differences from the Traub&Miles (1991) model.

See #329 (comment) for a thorough discussion.

@heplesser heplesser added T: Bug Wrong statements in the code or documentation ZC: Model DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next I: Behavior changes Introduces changes that produce different results for some users labels Apr 27, 2018
@heplesser heplesser added this to the NEST 2.16 milestone Apr 27, 2018
Copy link
Member

@Silmathoron Silmathoron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about this:

  • on the one hand the current behaviour is wrong and the fix is quite obviously better
  • on the other hand this feels horribly ad hoc and I feel we should rather try to improve the "spike detection" mechanism.

That being said, I am aware of the fact that it is very likely none of us has the time (nor probably any desire) to do this...
So unless we decide to remove the model altogether, I'll approve the PR ^^"

@heplesser
Copy link
Contributor Author

@Silmathoron This neuron model is used for the "hh_coba" benchmark in the Brette et al (2007) simulator review, so we ought to keep it in NEST. Unfortunately, the paper text does not describe spike or reset mechanisms at all. Benchmark code for different simulators is available on ModelDB, although it is not easy to infer how spike detection is done by the different simulators. The most transparent is Brian:

P=NeuronGroup(4000,model=eqs,
              threshold=EmpiricalThreshold(threshold=-20*mV,refractory=3*ms),
              implicit=True,freeze=True,compile=False)

I.e., a fixed threshold at -20 mV and an absolute refractory period of 3 ms. Since no reset is specified, I assume that without the refractory period, Brian would emit spikes for each time step until membrane potential drops below -20 mV.

I have not been able to find threshold conditions for other simulators, although for Neuron this piece of code

proc connect2target() { //$o1 target point process, $o2 returned NetCon
  soma $o2 = new NetCon(&v(1), $o1)
  $o2.threshold = -10 //*	
}

looks like there may be a spike threshold of 10 mV.

Quite a mess, but maybe compatibility with the Brian implementation would not be a bad idea?

@heplesser
Copy link
Contributor Author

See also #329.

@heplesser
Copy link
Contributor Author

See #329 (comment) for a thorough discussion of the situation and options.

@heplesser
Copy link
Contributor Author

@Silmathoron @diesmann I have updated the PR after discussions, so default t_ref now is 2 ms. I also made the connection to Brette et al (2007) clearer in the documentation. Could you take a look?

@heplesser heplesser changed the title Configure hh_cond_exp_traub with sensible t_ref. Better documentation of hh_cond_exp_traub and Brette-2007-compatible refractory time May 2, 2018
@heplesser heplesser added this to the NEST 2.16 milestone May 2, 2018
@Silmathoron
Copy link
Member

@heplesser so all the changes mentioned in #329 including the change in spike detection would be for NEST 3?

@heplesser
Copy link
Contributor Author

@Silmathoron Yes, they would all be for NEST 3 and then in a new model, while the current hh_cond_exp_traub would be kept primarily to keep the Brette et al (2007) examples runnning.

Copy link
Member

@Silmathoron Silmathoron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me 👍

@heplesser heplesser requested a review from suku248 May 28, 2018 09:52
Copy link
Contributor

@suku248 suku248 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks @heplesser

@heplesser heplesser removed the request for review from diesmann May 28, 2018 13:41
@heplesser heplesser merged commit d175510 into nest:master May 28, 2018
@heplesser heplesser deleted the fix-hh-cond-exp-traub-tref branch August 1, 2018 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Behavior changes Introduces changes that produce different results for some users S: High Should be handled next T: Bug Wrong statements in the code or documentation ZC: Model DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants