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

Fix GetConnections to correctly get connections where the target is a global receiving device #1251

Merged
merged 2 commits into from
Dec 3, 2019

Conversation

hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Jul 2, 2019

Fixes an issue with GetConnections, where it would not get the connection if the specified target is a global receiving device.

In jakobj#104, to improve the performance of GetConnections, targets are split into vectors containing neurons and devices. However, even though the global receiving device target is a device, it is connected in the same way as a normal neuron node. This means that the connection is never added to TargetTableDevices, and the connection isn't found.

This PR changes the splitting function to put global receiving devices in the neuron vector, which allows GetConnections to correctly get the connections. Two tests are also added, checking that GetConnections with specified source and target always gets the connection.

Fixes #1190.

To be able to get connections where the target is a globally receiving device.
Testing that GetConnections gets the connection with all possible types of node models specified as source or target.
@jougs jougs requested review from jakobj and suku248 July 22, 2019 10:22
@jougs jougs added ZC: Kernel DO NOT USE THIS LABEL I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Jul 22, 2019
@terhorstd terhorstd added this to the NEST 2.18.1 milestone Sep 2, 2019
Copy link
Contributor

@jakobj jakobj left a comment

Choose a reason for hiding this comment

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

looks good, thanks for this fix! 👍

@heplesser
Copy link
Contributor

@suku248 Could you review this PR in the near future? It is one of two PRs for 2.18.1 still open.

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!

@jougs jougs removed the request for review from suku248 December 3, 2019 13:40
@jougs jougs merged commit d59fdbb into nest:master Dec 3, 2019
@hakonsbm hakonsbm deleted the get_conns_global_receivers branch April 17, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetConnections() does not work with volume_transmitter as target
5 participants