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

Models no longer wrapped in SLI Module, model registration and thread-setting in managers revised #2990

Merged
merged 11 commits into from
Dec 13, 2023

Conversation

heplesser
Copy link
Contributor

The main purpose of this PR (developed by @jougs) is to register node and connection models directly in the ModelManager without wrapping them in a SLIModule. This change will allow implementation of extension module unloading at a later stage.

As a side effect of this change, this PR also:

  1. Creates one connection model instance per thread instead of storing the same pointer to a single model for each thread; thus common properties of connections can now be stored locally per thread.
  2. Changes the way managers are adjusted to a changed number of threads. In the past, the new number of threads was first set and then all managers finalized and initialized anew. Thus finalization of data structures with the old thread number was done with the new thread number already set, which can lead to problems. Now, all managers are first finalized (in opposite order of initialization), then the new thread number is set and then all managers are initialized. Both initialize() and finalize() now have a reset_kernel argument (default true) which controls if everything should be reset or only data structures adjusted to the new thread numbers. In the past, we actually reset more than we intended when just setting a new number of threads.

@heplesser heplesser added T: Enhancement New functionality, model or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 15, 2023
@heplesser heplesser added this to In progress in Kernel via automation Nov 15, 2023
@heplesser
Copy link
Contributor Author

@terhorstd @mlober Ping!

nestkernel/model_manager.h Outdated Show resolved Hide resolved
nestkernel/node_manager.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mlober mlober left a comment

Choose a reason for hiding this comment

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

The code compiles and runs nicely on my laptop. From what I can gather, the code and added documentation both look good to me (except for small typos - see comments).

@heplesser heplesser removed the request for review from jougs December 11, 2023 14:48
nestkernel/nest.h Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor Author

@terhorstd I have now committed the changes discussed in person today and addressed the two comments above. I have also merged in master.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

This looks OK from my perspective. There is obvious need for refactoring in some places, but that should not clutter this PR.
👍
Big thanks to everyone involved!

@heplesser heplesser merged commit f3e67a5 into nest:master Dec 13, 2023
24 checks passed
Kernel automation moved this from In progress to Done Dec 13, 2023
akorgor added a commit to jstapmanns/nest-simulator that referenced this pull request Dec 14, 2023
@terhorstd terhorstd added I: Behavior changes Introduces changes that produce different results for some users and removed I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jan 19, 2024
@heplesser heplesser deleted the models_no_module branch April 24, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Behavior changes Introduces changes that produce different results for some users S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants