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

Minor cleanup of code and comments #2980

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Minor cleanup of code and comments #2980

merged 5 commits into from
Oct 31, 2023

Conversation

jougs
Copy link
Contributor

@jougs jougs commented Oct 31, 2023

This is stuff that was lying in my cleanup branch and I don't want to be lost. See the list of commits for what and why.

@jougs jougs 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 Oct 31, 2023
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.

Hi @jougs! Thanks for these fixes. I agree with most of them, and also with moving register_* calls out of nestmodule.cpp. But I think the *_managers are not the right place for them: The managers take care of NEST parts, but they do not define which parts NEST has. With the new modelsmodule, this definition of what is part of NEST wrt neurons and synapses has become nicely separated, and ideally conn-builders and growth rules should be handled correspondingly. I see three options in no particular order and enumerated only for ease of reference:

  1. Leave the register_ calls where they are right now until we find a good place for them.
  2. Call register_conn_ from conn_builder.cpp and register_growth_ from growth_curve.cpp (provide registers components).
  3. Add the register_{conn,growth} calls to the auto-generated modelsmodule.cpp and maybe call that register_nest_components.cpp then.

@jougs
Copy link
Contributor Author

jougs commented Oct 31, 2023

@heplesser: I agree with all you say. Also, the code does not link properly as it stands. Given the situation I'm in, I'm afraid I won't be able to do anything about this anymore. Please feel free to take the changes, modify the code in whatever way you see fits, or discard them. I just wanted to get these off my chest.

@heplesser
Copy link
Contributor

@jougs If you could reset the connection_manager.cpp, nestmodule.cpp and sp_manager.cpp in this PR, I'd be happy to approve it :).

@jougs
Copy link
Contributor Author

jougs commented Oct 31, 2023

Done.

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.

@jougs Thanks!

@heplesser heplesser removed the request for review from terhorstd October 31, 2023 21:19
@heplesser
Copy link
Contributor

Minimal code tidying, thus merging based on single review.

@heplesser heplesser merged commit 806e7df into nest:master Oct 31, 2023
21 checks passed
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants