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

Make GetConnection filter by synapse label when only target attribute is given #1093

Closed
wants to merge 4 commits into from
Closed

Make GetConnection filter by synapse label when only target attribute is given #1093

wants to merge 4 commits into from

Conversation

stinebuu
Copy link
Contributor

This PR fixes #1085.

I have added a check for synapse label in ConnectionManager::get_connections for the case when GetConnection is called with only the target attribute.

@stinebuu stinebuu added T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL 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 Dec 13, 2018
@heplesser
Copy link
Contributor

@apdavison Could you review this as it should fix your #1085?

jakobj
jakobj reviewed Dec 20, 2018
View reviewed changes
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.

Thanks @stinebuu for fixing this quickly. I've added a question/comment inline.

nestkernel/connection_manager.cpp
or connections->get_syn_label( source_lcids[ i ] )
== synapse_label )
{
conns_in_thread.push_back( ConnectionDatum( ConnectionID(
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we use ConnectorBase::get_connection here? this would also take care of filtering by label and would make this if branch consistent with the others. in addition we wouldn't need to introduce the helper functions below. is speed an issue here?

This was referenced Jan 8, 2019
Merged
Merged
@stinebuu
Copy link
Contributor Author

stinebuu commented Jan 9, 2019

I am closing this in favor of #1099, which, in addition to fixing #1085, also improves performance.

@stinebuu stinebuu closed this Jan 9, 2019
@stinebuu stinebuu deleted the fix-1085_GetConn_filter branch January 9, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobj jakobj jakobj left review comments

@hakonsbm hakonsbm Awaiting requested review from hakonsbm

Assignees
No one assigned
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GetConnections does not seem to filter by synapse label when only the target attribute is given.
3 participants
@stinebuu @heplesser @jakobj