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 description of rate model neurons and change parameter names #1050

Merged
merged 7 commits into from
Dec 10, 2018

Conversation

janhahne
Copy link
Contributor

Some questions on the NEST user mailing list indicated shortcomings in the documentation of the rate-based models in NEST.

The main problems:

  • Exact form of the rate model equations is unclear
  • mean input und variance of the rate neuron (in the absence of input from the network) are named "mean" and "std" and are described as properties of the gaussian white noise rather than of the rate neuron itself.

Solutions in the PR:

  • Added formulas of the models to the base class documentation
  • renamed "mean" and "std" to "mu" and "sigma" to avoid the confusing names and to be consistent with the original paper
  • updated the descriptions, such that "mu" and "sigma" are understood as parameters of the rate neuron (rather than of the noise)

In order to not break current rate model scripts (especially the just recently released ReScience paper about the Grossberg model) GetStatus and SetStatus still also accept the old names. SetStatus however kindly reminds you to use the new names in the future.

@janhahne
Copy link
Contributor Author

@ddahmen could you have a look?

Copy link
Contributor

@ddahmen ddahmen left a comment

Choose a reason for hiding this comment

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

Thanks for making the documentation more self-explainatory and consistent with Hahne et al. (2017).
Looks good!

@stinebuu stinebuu added ZC: Model DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Oct 25, 2018
@heplesser heplesser changed the title Updated description of rate model neurons Updated description of rate model neurons and changed parameter names Oct 29, 2018
@heplesser heplesser added I: User Interface Users may need to change their code due to changes in function calls and removed I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Oct 29, 2018
@heplesser heplesser changed the title Updated description of rate model neurons and changed parameter names Update description of rate model neurons and change parameter names Oct 29, 2018
@clinssen
Copy link
Contributor

Hi @janhahne, thanks for improving the documentation and naming! Just a few small points.

Could you include "@BeginDocumentation" in the comments block for rate_neuron_ipn and _opn? This ensures they get picked up by the automatic documentation generation tools.

In the ASCII-art equations, the lambda is supposed to be there? I cannot immediately find the corresponding equation in the paper. Could you perhaps add a reference to the corresponding equation number in the paper?

I'm not sure how I feel about the ASCII-art style: I'm also one of those terrible people who uses unicode for variable names, but this a bit of a mixed bag (e.g. you use w^{< 0} (with LaTeX-style brackets) but also d_ij (without brackets). Perhaps we can stick to pure LaTeX? We will probably be able to nicely render this using the automatic documentation generation tools, so it comes out nicely in HTML.

Thanks!

@janhahne
Copy link
Contributor Author

@clinssen Thanks for the review! I have added the @BeginDocumentation keyword to the documentation of the base classes and adjusted the formatting.

Regarding lambda: Lambda is an additional parameter that was added after the release of the paper. Indeed the entire framework for rate models was extended quite a bit with PR #858. This is one of the main reasons why it is important to add the equations to the source code documentation.

Regarding the ASCII-art style: I totally agree that this is not optimal. I tried pure LaTeX at first, but unfortunately it is almost impossible to read (and to understand) for this rather complex equations. This is why a went with a mixture that (in my view) provides the best readability. An automatic conversion of LaTeX code into nicely readable equations in the HTML helpfiles would be great, but is something like this already in place? Otherwise I would suggest to keep it this way for now and to change it to pure LaTeX once this is in place. What do you think?

@clinssen
Copy link
Contributor

@jessica-mitchell: could you comment on the preferred maths style in source code comment blocks?

@jessica-mitchell
Copy link
Contributor

@clinssen @janhahne We've looked at doing this with doxygen-sphinx and it can render latex math with mathjax nicely in html, so I would be in favour of latex. This is not yet in place for models but we've tested it out and I think we have a decent solution we'd like to implement. Although to be fair, I haven't looked into other math formats.

@janhahne
Copy link
Contributor Author

@jessica-mitchell @clinssen That sounds like a good and helpful solution. I converted the formulas to plain latex. Once the rendering of latex is in place we could think of improving the readability of the resulting latex formula by adding additional spacing/splitting and brackets in different sizes via \left[ and \right]. But for now the "source code" should be readable as well. @clinssen please have another look if you are satisfied.

Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

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

Many thanks!

@janhahne
Copy link
Contributor Author

janhahne commented Nov 27, 2018

@terhorstd There are two approving reviews already, do you still want to review this (as requested by yourself some while ago)? Otherwise this should be ready for merging :-)

@stinebuu stinebuu merged commit 8e52360 into nest:master Dec 10, 2018
@janhahne janhahne deleted the update_rate_docu branch December 10, 2018 13:29
janhahne added a commit to janhahne/nest-simulator that referenced this pull request Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: User Interface Users may need to change their code due to changes in function calls S: High Should be handled next T: Maintenance Work to keep up the quality of the code and 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.

6 participants