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

Update documentation and refactor comments in iaf_psc_exp current input #2229

Merged
merged 15 commits into from
Jan 17, 2022

Conversation

clinssen
Copy link
Contributor

@clinssen clinssen commented Dec 6, 2021

Fixes #1226.

The meaning of the i_syn_exp_ was left unchanged, but a warning about its meaning was added in two places in the documentation.

@@ -248,8 +248,6 @@ nest::iaf_psc_exp::calibrate()

const double h = Time::get_resolution().get_ms();

// numbering of state vaiables: i_0 = 0, i_syn_ = 1, V_m_ = 2

// commented out propagators: forward Euler
Copy link
Contributor

Choose a reason for hiding this comment

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

The Euler propagators should go. If at all, I would add a remark to the model documentation pointing out that this neuron model differs from the implemented in [1]_ by using exact integration instead of forward Euler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment says:

  // commented out propagators: forward Euler
  // needed to exactly reproduce Tsodyks network

Perhaps it makes sense for people coming after us to leave this in? We could also wrap it in some preprocessor conditions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am strictly against leaving commented-out code in. Anyone with a minimal understanding of Euler's method should be able to re-Eulerize the code if they want to (we should put a remark into the model doc). Compiler switches are too cumbersome, since we'd need to propagate them from the CMake level. The only alternative I would consider would be a runtime switch in the model, similar to the consistent_integration switch in the Izhikevich model.

@heplesser heplesser added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Dec 7, 2021
@heplesser heplesser added this to To do in Models via automation Dec 7, 2021
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of phrases to clear up

models/iaf_psc_exp.h Outdated Show resolved Hide resolved
models/iaf_psc_exp.h Outdated Show resolved Hide resolved
models/iaf_psc_exp.h Outdated Show resolved Hide resolved
models/iaf_psc_exp.h Outdated Show resolved Hide resolved
models/iaf_psc_exp.h Outdated Show resolved Hide resolved
models/iaf_psc_exp.h Outdated Show resolved Hide resolved
models/iaf_psc_exp.h Outdated Show resolved Hide resolved
C.A.P. Linssen and others added 12 commits January 3, 2022 11:42
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
@clinssen
Copy link
Contributor Author

clinssen commented Jan 3, 2022

@heplesser, @jessica-mitchell: thanks for the comments! I updated the code, could you have another look?

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@clinssen thanks! lgtm

@heplesser
Copy link
Contributor

@clinssen Could you merge master into this PR to benefit from the fix in #2253 for the failing Python tests? Once all tests pass, this one is good to be merged.

@terhorstd terhorstd merged commit 0dd1bff into nest:master Jan 17, 2022
@clinssen clinssen moved this from To do to Done in Models Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Models
  
Done
Development

Successfully merging this pull request may close these issues.

Review iaf_psc_exp current input
4 participants