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

Completely revised iaf_psc_alpha user documentation #2856

Merged
merged 20 commits into from
Sep 13, 2023

Conversation

heplesser
Copy link
Contributor

This PR provides completely new version of the user documentation of iaf_psc_alpha with a focus on the equations underlying the model. I provide this as a suggestion for how to re-write model documentation. It can probably do with a bit of polishing before being used as template for other models.

@heplesser heplesser added T: Discussion Still searching for the right way to proceed / suggestions welcome S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jul 7, 2023
@heplesser heplesser added this to In progress in Documentation via automation Jul 7, 2023
@jessica-mitchell
Copy link
Contributor

Also relates to issue #2464

models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
Documentation automation moved this from In progress to Review Jul 10, 2023
environment.yml Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
models/iaf_psc_alpha.h Show resolved Hide resolved
models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
heplesser and others added 11 commits July 15, 2023 23:45
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: Jochen Martin Eppler <jougs@gmx.net>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: Jan-Eirik Welle Skaar <jewskaar@gmail.com>
@heplesser
Copy link
Contributor Author

heplesser commented Aug 3, 2023

@jessica-mitchell @jougs @clinssen @janskaar Thank you for your comments, I have followed up almost all of them. Please have a look.

I am struggling with one problem now: The parameter table in the comment is 129 characters wide, so clang-format complains. Is there any way to either suppress clang-format for this comment or break lines in a RST table spec?

https://github.com/heplesser/nest-simulator/blob/1e203c9e5c9ac914ea59f640b1bc185217c35c5d/models/iaf_psc_alpha.h#L137

@nicolossus
Copy link
Member

I am struggling with one problem now: The parameter table in the comment is 129 characters wide, so clang-format complains. Is there any way to either suppress clang-format for this comment or break lines in a RST table spec?

@heplesser I think you can disable and re-enable clang-format like this:

// clang-format off
/*
<docs>
*/
// clang-format on

This will, however, suppress clang-format for the all the documentation. I don't think it is possible to suppress the table only.

@heplesser
Copy link
Contributor Author

I am struggling with one problem now: The parameter table in the comment is 129 characters wide, so clang-format complains. Is there any way to either suppress clang-format for this comment or break lines in a RST table spec?

@heplesser I think you can disable and re-enable clang-format like this:

// clang-format off
/*
<docs>
*/
// clang-format on

This will, however, suppress clang-format for the all the documentation. I don't think it is possible to suppress the table only.

Thanks, this worked!

models/iaf_psc_alpha.h Outdated Show resolved Hide resolved
@jougs
Copy link
Contributor

jougs commented Aug 22, 2023

While I generally agree with many things said here to improve the model documentation, I also feel we should not overthink (and overdo) this by putting too much effort into adding new things.

In my opinion, this PR should rather serve as a playground and result in a solid template that can be ported to the NESTML documentation generation pipeline. This is where the magic happens now and improvements there would immediately benefit all models (c.f. model equations, units and defaults for parameters, model characterization figures in this example).

In the last three years we only gained 3 new models in NEST (jonke_synapse, cm_default, spike_injector) and I would rather put some increased effort into a better integration of NESTML into the build process of NEST and let all models be generated instead of fixing equations and parameters in manually written model documentation.

@clinssen, @poojanbabu, @jessica-mitchell.

Just my 2 cents.

@heplesser
Copy link
Contributor Author

@jougs I fully agree with you. I now have one question concerning your earlier comments vs the playground perspective. I entirely agree that it would be useful to link terms in the documentation to glossary entries. But I fear it would be at least another two weeks until I will get around to creating the glossary entries. Would you be ok with merging this now and creating a follow-up issue for glossaryfication?

@heplesser
Copy link
Contributor Author

@clinssen @jessica-mitchell Could you re-review?

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.

thanks lgtm!

@clinssen
Copy link
Contributor

You define I_syn,ex but not I_syn,in. Perhaps you could use the notation I_syn,x or just mention that I_syn,in is defined similarly to I_syn,ex. I am otherwise happy with the changes. Much obliged!

@heplesser
Copy link
Contributor Author

You define I_syn,ex but not I_syn,in. Perhaps you could use the notation I_syn,x or just mention that I_syn,in is defined similarly to I_syn,ex. I am otherwise happy with the changes. Much obliged!

Thanks for catching this, @clinssen! It should now be fixed.

@heplesser
Copy link
Contributor Author

@clinssen Ping!

@heplesser heplesser merged commit cc88f11 into nest:master Sep 13, 2023
13 checks passed
Documentation automation moved this from Review to Done Sep 13, 2023
@terhorstd terhorstd added T: Maintenance Work to keep up the quality of the code and documentation. and removed T: Discussion Still searching for the right way to proceed / suggestions welcome labels Sep 15, 2023
@heplesser heplesser deleted the model_doc branch September 15, 2023 18:08
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: Maintenance Work to keep up the quality of the code and documentation.
Projects
Documentation
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants