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

Refractory tests and corrected hh_cond_exp_traub (fixes #473) #590

Merged
merged 14 commits into from Jan 11, 2017

Conversation

Projects
None yet
3 participants
@Silmathoron
Contributor

Silmathoron commented Dec 13, 2016

This PR adresses #473 by correcting the refractory behaviour of hh_cond_exp_traub
It also implements a random refractory test for many hybrid models.

Show outdated Hide outdated pynest/nest/tests/test_refractory.py
"aeif_cond_beta_multisynapse",
"amat2_psc_exp",
"ginzburg_neuron",
"hh_cond_exp_traub",

This comment has been minimized.

@heplesser

heplesser Dec 13, 2016

Contributor

Why do you need to ignore this model? Isn't that the one you are fixing in this PR?

@heplesser

heplesser Dec 13, 2016

Contributor

Why do you need to ignore this model? Isn't that the one you are fixing in this PR?

This comment has been minimized.

@Silmathoron

Silmathoron Dec 13, 2016

Contributor

Yes, but the hh_* models do not clamp the potential, so I cannot easily check the refractory period... I'll try using a huge current

@Silmathoron

Silmathoron Dec 13, 2016

Contributor

Yes, but the hh_* models do not clamp the potential, so I cannot easily check the refractory period... I'll try using a huge current

This comment has been minimized.

@Silmathoron

Silmathoron Dec 14, 2016

Contributor

No, I think there is just no way to check the HH models (I'm not even sure t_ref makes any sense for them)

@Silmathoron

Silmathoron Dec 14, 2016

Contributor

No, I think there is just no way to check the HH models (I'm not even sure t_ref makes any sense for them)

@heplesser

Looks good, but I wonder if you really need to exclude all the models you excluded, see comments below.

Show outdated Hide outdated pynest/nest/tests/test_refractory.py
ignore_model = [
"aeif_cond_alpha_RK5",
"aeif_cond_alpha_multisynapse",
"aeif_cond_beta_multisynapse",

This comment has been minimized.

@heplesser

heplesser Dec 13, 2016

Contributor

Why do you need to exclude the multisynapse models?

@heplesser

heplesser Dec 13, 2016

Contributor

Why do you need to exclude the multisynapse models?

This comment has been minimized.

@Silmathoron

Silmathoron Dec 13, 2016

Contributor

RK5's implementation of t_ref is still wrong and I was waiting for you to merge your PR regarding the other two ;)
Now that it's done, I'll include them in the tests tomorrow.

@Silmathoron

Silmathoron Dec 13, 2016

Contributor

RK5's implementation of t_ref is still wrong and I was waiting for you to merge your PR regarding the other two ;)
Now that it's done, I'll include them in the tests tomorrow.

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Dec 14, 2016

Contributor

Ok, I corrected the test (and recorrected it when I remembered why I had built the list of neurons to be tested that way... -_-')
I think this is the best I can do, but please tell me if you see how it might be enhanced.

Contributor

Silmathoron commented Dec 14, 2016

Ok, I corrected the test (and recorrected it when I remembered why I had built the list of neurons to be tested that way... -_-')
I think this is the best I can do, but please tell me if you see how it might be enhanced.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 16, 2016

Contributor

@Silmathoron Very nice. But could you do one more update? The Hill-Tononi revision has just been merged this morning. Could you merge that into your branch and make sure that your test still works?

Contributor

heplesser commented Dec 16, 2016

@Silmathoron Very nice. But could you do one more update? The Hill-Tononi revision has just been merged this morning. Could you merge that into your branch and make sure that your test still works?

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Dec 16, 2016

Contributor

This is what I did this morning just after abigail merged it ;)

Contributor

Silmathoron commented Dec 16, 2016

This is what I did this morning just after abigail merged it ;)

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 16, 2016

Contributor

Sorry, my mistake. Github had listed all your recent commits under "... added some commits 2 days ago", so I thought it couldn't contain the revised HT.

Contributor

heplesser commented Dec 16, 2016

Sorry, my mistake. Github had listed all your recent commits under "... added some commits 2 days ago", so I thought it couldn't contain the revised HT.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 16, 2016

Contributor

@jschuecker Could you be the second reviewer for this one? It is just a small (but important fix) and a new test.

Contributor

heplesser commented Dec 16, 2016

@jschuecker Could you be the second reviewer for this one? It is just a small (but important fix) and a new test.

@heplesser heplesser added this to the NEST 2.12 milestone Jan 9, 2017

Show outdated Hide outdated pynest/nest/tests/test_refractory.py
# --------------------------------------------------------------------------- #
# Simulation and refractory time
# -------------------------

This comment has been minimized.

@jschuecker

jschuecker Jan 10, 2017

Contributor

you could change this to "refractory time limits" to avoid confusion as this are only the lower and upper bounds right?

@jschuecker

jschuecker Jan 10, 2017

Contributor

you could change this to "refractory time limits" to avoid confusion as this are only the lower and upper bounds right?

This comment has been minimized.

@Silmathoron

Silmathoron Jan 11, 2017

Contributor

Done (and additional comments added)

@Silmathoron

Silmathoron Jan 11, 2017

Contributor

Done (and additional comments added)

Show outdated Hide outdated pynest/nest/tests/test_refractory.py
else:
Vr = nest.GetStatus(neuron, "V_reset")[0]
times = nest.GetStatus(vm, "events")[0]["times"]
idx_max = (np.argwhere(np.isclose(times, spike_times[1]))[0][0]

This comment has been minimized.

@jschuecker

jschuecker Jan 10, 2017

Contributor

is there a reason why np.isclose is used here? Otherwise you could make this consistent with line 209.

@jschuecker

jschuecker Jan 10, 2017

Contributor

is there a reason why np.isclose is used here? Otherwise you could make this consistent with line 209.

This comment has been minimized.

@Silmathoron

Silmathoron Jan 11, 2017

Contributor

Yes, it's for precise models ;) I'll add a comment
EDIT: or maybe not anymore... I'll check that!
EDIT 2: corrected

@Silmathoron

Silmathoron Jan 11, 2017

Contributor

Yes, it's for precise models ;) I'll add a comment
EDIT: or maybe not anymore... I'll check that!
EDIT 2: corrected

@jschuecker

This comment has been minimized.

Show comment
Hide comment
@jschuecker

jschuecker Jan 10, 2017

Contributor

@Silmathoron: Very nice new test! I only have two minor suggestions (see my inline comments).

Contributor

jschuecker commented Jan 10, 2017

@Silmathoron: Very nice new test! I only have two minor suggestions (see my inline comments).

Show outdated Hide outdated pynest/nest/tests/test_refractory.py
@@ -134,14 +134,14 @@
# --------------------------------------------------------------------------- #
# Simulation and refractory time
# Simulation and refractory time limits

This comment has been minimized.

@jschuecker

jschuecker Jan 11, 2017

Contributor

@Silmathoron, Sorry, I meant to change this to "Simulation time and refractory time limits"

@jschuecker

jschuecker Jan 11, 2017

Contributor

@Silmathoron, Sorry, I meant to change this to "Simulation time and refractory time limits"

This comment has been minimized.

@Silmathoron

Silmathoron Jan 11, 2017

Contributor

easy ;)

@Silmathoron

Silmathoron Jan 11, 2017

Contributor

easy ;)

@jschuecker

This comment has been minimized.

Show comment
Hide comment
@jschuecker

jschuecker Jan 11, 2017

Contributor

@Silmathoron thanks for the corrections. 👍 from my side

Contributor

jschuecker commented Jan 11, 2017

@Silmathoron thanks for the corrections. 👍 from my side

@heplesser heplesser merged commit f9c3748 into nest:master Jan 11, 2017

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