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

Make kernel parameter documentation in kernel and PyNEST consistent #2469

Merged

Conversation

JanVogelsang
Copy link
Contributor

Fixes #1554

Added all missing parameters in the kernel_manager's docstring, made the whole docstring consistent and fixed the parameter order (alphabetical order within groups).

@jessica-mitchell jessica-mitchell added this to In progress in Documentation via automation Sep 15, 2022
@jessica-mitchell jessica-mitchell added this to In progress in PyNEST via automation Sep 15, 2022
@jessica-mitchell jessica-mitchell added S: Normal Handle this with default priority 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 Sep 15, 2022
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 @JanVogelsang for the update. I think there is one parameter missing and that's kernel_status. Could you please add it?

Documentation automation moved this from In progress to Review Sep 20, 2022
PyNEST automation moved this from In progress to Review Sep 20, 2022
@jessica-mitchell jessica-mitchell changed the title Made kernel parameter documentation in kernel and PyNEST consistent Make kernel parameter documentation in kernel and PyNEST consistent Sep 20, 2022
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.

lgtm!

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@JanVogelsang Thank you for working through this! I added a number of suggestions on details.

I then have a general question @jessica-mitchell: Within each group of parameters, parameters are currently ordered alphabetically. This leads to quite some bit of mixing, e.g., of min_, max_ parameters. I wonder if it would not make more sense to order parameters logically also within groups. I also wonder if we should not make more of an effort to move doc on parameters that users will inspect or change frequently forward and those only interesting for power users/developers towards the end.

nestkernel/kernel_manager.h Show resolved Hide resolved
nestkernel/kernel_manager.h Outdated Show resolved Hide resolved
nestkernel/kernel_manager.h Outdated Show resolved Hide resolved
nestkernel/kernel_manager.h Show resolved Hide resolved
nestkernel/kernel_manager.h Show resolved Hide resolved
nestkernel/kernel_manager.h Show resolved Hide resolved
nestkernel/kernel_manager.h Outdated Show resolved Hide resolved
nestkernel/kernel_manager.h Outdated Show resolved Hide resolved
nestkernel/kernel_manager.h Show resolved Hide resolved
nestkernel/kernel_manager.h Outdated Show resolved Hide resolved
@JanVogelsang
Copy link
Contributor Author

It might make sense to have a different grouping (or any groups at all) in the PyNest API documentation compared to the docstring in the source code (which probably only developers will ever get to see).

Should I also apply all your changes to the PyNest documentation as well (which is based on a different file)?

@jessica-mitchell
Copy link
Contributor

I then have a general question @jessica-mitchell: Within each group of parameters, parameters are currently ordered alphabetically. This leads to quite some bit of mixing, e.g., of min_, max_ parameters. I wonder if it would not make more sense to order parameters logically also within groups. I also wonder if we should not make more of an effort to move doc on parameters that users will inspect or change frequently forward and those only interesting for power users/developers towards the end.

@heplesser We could certainly look at moving these around; I am not familiar enough with some of the functions to know which ones would be considered more developer-leaning. Would the entire nestModule class be considered something to be moved lower (or another page?). Or is it more fine-grained than that?

@heplesser
Copy link
Contributor

We should discuss this in the next NEST Developer VC. It is unfortunate to have the same information twice in ever so slightly different format. I hope we can find a way to have it only once and then just extract it in sensible ways.

@heplesser
Copy link
Contributor

@JanVogelsang In the meantime, would you accept my suggestions (if you agree with them) and run clang-format on the file to get the formatting right?

@JanVogelsang
Copy link
Contributor Author

@heplesser I reviewed all your requests and accepted most of them. I changed some of them to keep up a consistent formatting by replacing commas by semicolons inside parentheses, when they are not used within a sentence/block. If you prefer commas everywhere instead, I can also replace all semicolons by commas, I just want to make sure I use either of both consistently.

@heplesser
Copy link
Contributor

Thanks @JanVogelsang! In spite of my comments above, I suggest to merge this PR now once all tests pass, since it provides an improvement. We can then handle the ordering of parameters and possible integration between kernel and PyNEST in a separate PR.

@heplesser heplesser merged commit 6286ea7 into nest:master Sep 22, 2022
Documentation automation moved this from Review to Done Sep 22, 2022
PyNEST automation moved this from Review to Done Sep 22, 2022
@JanVogelsang JanVogelsang deleted the issue-1554_kernel_parameter_documentation branch September 22, 2022 13:11
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
PyNEST
  
Done
Development

Successfully merging this pull request may close these issues.

Make kernel parameter documentation in kernel and PyNEST consistent
3 participants