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 spatial attribute positions of sliced NodeCollection #2415

Merged
merged 6 commits into from
Aug 31, 2022

Conversation

hakonsbm
Copy link
Contributor

For internal consistency and efficiency, metadata of nodes in a sliced NodeCollection points to the metadata of the original NodeCollection. When getting the positions in the spatial parameter of a sliced NodeCollection, using nodes.spatial['positions'], positions for all nodes in the original NodeCollection is therefore returned. This PR adds a filter on the returned positions to only get positions of nodes in the sliced NodeCollection, which fixes #2414.

@hakonsbm hakonsbm added T: Bug Wrong statements in the code or documentation S: Critical Needs to be addressed immediately I: User Interface Users may need to change their code due to changes in function calls labels Jun 21, 2022
@terhorstd terhorstd added the need reviewer Ready to be reviewed but no reviewer assigned label Aug 2, 2022
@terhorstd terhorstd added this to the NEST 3.4 milestone Aug 2, 2022
@heplesser heplesser requested review from nicolossus, ackurth and mlober and removed request for ackurth August 18, 2022 08:03
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.

The test only creates a single neuron when it should probably create multiple neurons and then create a sliced NC from that.

nestkernel/nest.h Show resolved Hide resolved
@hakonsbm
Copy link
Contributor Author

@mlober @nicolossus Thanks for your reviews. I've updated with the suggested changes. Please have another look.

Copy link
Member

@nicolossus nicolossus 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 the changes!

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 the changes. I still think the node collection object should be sliced first for the test, since now the NC is a standard one with size 10.
I think it would work if you added the line
nodes = nodes[::2] in the code.
If I am misunderstanding, please explain.

@hakonsbm
Copy link
Contributor Author

@mlober The test already checks positions of a NC sliced to a single node. The positions of each node are gathered to a list that can be compared to the original positions:

all_positions = sum([list(nodes[i].spatial['positions']) for i in range(len(nodes))], start=[])

However, I've added a check of positions sliced with nodes[::2] as well now.

@hakonsbm hakonsbm requested a review from mlober August 29, 2022 07:46
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.

I see, thanks!

@heplesser heplesser merged commit ff0ac34 into nest:master Aug 31, 2022
@hakonsbm hakonsbm deleted the fix_sliced_positions branch August 31, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: User Interface Users may need to change their code due to changes in function calls need reviewer Ready to be reviewed but no reviewer assigned S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation
Projects
Status: Done
5 participants