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

Set sender node id info with correct target lcid #2665

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

JanVogelsang
Copy link
Contributor

@JanVogelsang JanVogelsang commented Apr 20, 2023

In the event_delivery_manager the sender_node_id_info is set during the event delivery using the same lcid for all contiguous connections with the same source instead of the actual lcid of the connection. This is not wrong per se, but prevents us from grabbing the correct lcid lateron from the set_sender_node_id_info. One could argue if that shouldn't be done at all, but why not. In NEST there is currently no use case in the kernel or any existing model, but it might potentially be used in NESTML or when users write their own models.
Setting the correct lcid does not add any overhead and even reduces some redundancy in the code.

@JanVogelsang JanVogelsang added ZC: Kernel DO NOT USE THIS LABEL S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Apr 20, 2023
@JanVogelsang JanVogelsang self-assigned this Apr 20, 2023
@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Jun 20, 2023
@terhorstd terhorstd added T: Maintenance Work to keep up the quality of the code and documentation. and removed ZC: Kernel DO NOT USE THIS LABEL labels Jun 27, 2023
@jougs
Copy link
Contributor

jougs commented Jun 29, 2023

@JanVogelsang: can you please comment on the status of this (and maybe merge master)? Thanks!

@github-actions github-actions bot removed the stale Automatic marker for inactivity, please have another look here label Jun 30, 2023
@JanVogelsang
Copy link
Contributor Author

Well, status is that I am waiting for a review. It's a rather small change and even a non-breaking one, but one that might help reduce errors in the future (as I had a bug in one of my branches because of this).

@JanVogelsang JanVogelsang reopened this Jul 1, 2023
@jougs jougs requested review from clinssen and removed request for heplesser and abigailm July 10, 2023 10:32
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.

Many thanks for this fix.

nestkernel/event_delivery_manager.cpp Show resolved Hide resolved
@jougs jougs requested a review from mlober July 12, 2023 12:52
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.

Thanks for this improvement. Looks good to me.

@abigailm abigailm merged commit b9514cc into nest:master Jul 18, 2023
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
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants