Skip to content

Filter connections also by target when dumping layer connections#2920

Merged
jougs merged 13 commits intonest:masterfrom
nicolossus:fix_DumpLayerConnections
Sep 7, 2023
Merged

Filter connections also by target when dumping layer connections#2920
jougs merged 13 commits intonest:masterfrom
nicolossus:fix_DumpLayerConnections

Conversation

@nicolossus
Copy link
Member

Fixes #2629.

The main purpose of this PR is to fix the bug reported in #2629. The reason for the bug was that the internal kernel call to get_connections did not filter by the provided target NodeCollection.

The PR also introduces some minor changes regarding more informative variable names and declaring the appropriate data type. More majorly, the find functions of node_collection.h are renamed to get_lid to better reflect that the functions return an index and not an iterator (the find function for a container typically returns an iterator).

The final addition of this PR is that path-like objects are now allowed to be passed as filename to DumpLayerConnections.

@nicolossus nicolossus added T: Bug Wrong statements in the code 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 Sep 4, 2023
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.

Thanks @nicolossus. I just have one question and suggestion concerning the test.

@nicolossus nicolossus requested a review from heplesser September 5, 2023 22:20
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.

@nicolossus Thank you! I will approve this now, but am slightly irritated that we needed the stringify_path() to support SLI shortly before SLI will be a matter of the past. Could you create a follow up issue to remove it again? An maybe create another follow-up issue to remind us to see if we want to convert all path handling to pathlib.

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.

This looks good to me. I guess we can handle the removal of the stringify_path function in the merge to new-pynest.

@jougs jougs merged commit 753febe into nest:master Sep 7, 2023
@nicolossus nicolossus deleted the fix_DumpLayerConnections branch October 29, 2023 10:05
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: High Should be handled next T: Bug Wrong statements in the code or documentation

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

DumpLayerConnections fails when connecting source layer to more than one target layer

3 participants

Comments