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.

Sign up for GitHub

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

Jump to bottom

Move connection flags to Connection static const member #2517

Merged
merged 18 commits into from
Jan 25, 2023
Merged

Move connection flags to Connection static const member #2517

merged 18 commits into from
Jan 25, 2023

Conversation

clinssen
Copy link
Contributor

A Connection knows best about its own required connection flags. These flags are therefore moved from the ModelsModule, where each individual model is registered with the kernel through a register_connection_model() call, to the Connection class and its children (such as the clopath_synapse) as a new member const static RegisterConnectionModelFlags flags. Clopath and Urbanczik models initialize them using their own flags, OR-ed with the default flags in the base class.

@jougs: should we keep the non-const operator| on our custom Enum type, or ditch it?

If you are happy with how this looks, I will make the same changes for the secondary connection flags.

@clinssen clinssen requested a review from jougs October 25, 2022 14:15
@clinssen clinssen marked this pull request as draft October 25, 2022 14:15
@heplesser heplesser self-requested a review October 26, 2022 07:25
@heplesser
Copy link
Contributor

@clinssen This seems like a good idea. Can you merge master to make macOS pass again (see #2515)?

jougs
jougs reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Very nice :-)

I have added some inline comments to further improve the encapsulation of the flags.

nestkernel/connection.h Outdated Show resolved Hide resolved
nestkernel/model_manager_impl.h Outdated Show resolved Hide resolved
heplesser
heplesser reviewed Oct 26, 2022
View reviewed changes
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.

@clinssen This looks very interesting. I agree with @jougs' comments and they inspired me to some more suggestions.

libnestutil/enum_bitfield.h Outdated Show resolved Hide resolved
nestkernel/connection.h Outdated Show resolved Hide resolved
@jougs
Copy link
Contributor

jougs commented Nov 10, 2022

@clinssen: I know you are busy, but still: any news on this?!

@clinssen clinssen marked this pull request as ready for review November 11, 2022 18:38
@clinssen
Copy link
Contributor Author

Thank you for the reviews! The new changes should address all of the comments raised.

heplesser
heplesser previously requested changes Nov 14, 2022
View reviewed changes
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.

@clinssen This looks so much nicer now that it inspired me to some ideas to make it even tidier :).

models/rate_connection_instantaneous.h Outdated Show resolved Hide resolved
nestkernel/connector_model.h Outdated Show resolved Hide resolved
nestkernel/connector_model.cpp Outdated Show resolved Hide resolved
nestkernel/connector_model.h Outdated Show resolved Hide resolved
nestkernel/connector_model.h Outdated Show resolved Hide resolved
nestkernel/connector_model.h Outdated Show resolved Hide resolved
nestkernel/connector_model.h Outdated Show resolved Hide resolved
libnestutil/enum_bitfield.h Outdated Show resolved Hide resolved
@jougs
Copy link
Contributor

jougs commented Jan 16, 2023

@clinssen: I have just sent you clinssen#6 to fix the problem with the failing assertions. Locally, I still see some tests failing and I can look into this hopefully tomorrow.

jougs
jougs reviewed Jan 16, 2023
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

I second most of @heplesser's comments and have added some additional ones.

nestkernel/connector_model.h Outdated Show resolved Hide resolved
nestkernel/connector_model.h Outdated Show resolved Hide resolved
@clinssen clinssen mentioned this pull request Jan 17, 2023
Merged
@terhorstd terhorstd added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know labels Jan 17, 2023
@terhorstd terhorstd added this to PRs in progress in Kernel via automation Jan 17, 2023
@clinssen
Copy link
Contributor Author

@jougs, @heplesser: thanks very much for your input! The latest commits should address all of your comments exactly as you proposed them, so there are no unresolved conversations remaining. Could you give it another look (or seal of approval)? Cheers!

jougs
jougs requested changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Only some minor remarks left.

models/gap_junction.h Outdated Show resolved Hide resolved
nestkernel/conn_builder.cpp Show resolved Hide resolved
nestkernel/connection_manager.cpp Outdated Show resolved Hide resolved
Kernel automation moved this from PRs in progress to PRs pending approval Jan 18, 2023
jougs
jougs approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Thanks!

@jougs jougs requested review from med-ayssar and removed request for heplesser January 18, 2023 17:03
@jougs
Copy link
Contributor

jougs commented Jan 23, 2023

@heplesser, @med-ayssar: could one of you please have a quick look? I think this is good to go. Thanks!

med-ayssar
med-ayssar approved these changes Jan 25, 2023
View reviewed changes
nestkernel/model_manager.h Show resolved Hide resolved
@jougs jougs dismissed heplesser’s stale review January 25, 2023 14:19

We meanwhile have a positive review by @med-ayssar

@jougs jougs merged commit b5f337d into nest:master Jan 25, 2023
Kernel automation moved this from PRs pending approval to Done (PRs and issues) Jan 25, 2023
@pnbabu pnbabu mentioned this pull request Jan 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jougs jougs jougs approved these changes

@med-ayssar med-ayssar med-ayssar approved these changes

@heplesser heplesser heplesser left review comments

Assignees
No one assigned
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@clinssen @heplesser @jougs @med-ayssar @terhorstd