Skip to content

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
Copy link
Contributor Author

@terhorstd @mlober Ping!

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
@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.

@heplesser heplesser requested a review from terhorstd December 11, 2023 21:44
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
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

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants