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

ContiniousSpace: Speedup get_neighbors by using KDTree #1619

Closed
wants to merge 2 commits into from

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Mar 21, 2023

Speedup the get_neighbors() function in ContiniousSpace by using a SciPy KDTree. It improves performance especially with a large number of agents. For small number of agents, the performance is roughly the same, without large regressions.

I've run some benchmarks on the boid_flockers model, with different number of agents, measuring the total runtime in seconds.

boxplot

Please review carefully, both changes in functionality/behaviour as performance.

EwoutH added 2 commits March 21, 2023 12:29
Speedup the get_neighbors() function in ContiniousSpace by using a KDTree. It improves performance especially with a large number of agents. For small number of agents, the performance is roughly the same, without large regressions.
It's now needed for the KDTree functionality in the get_neighbors function of ContiniousSpace.
@EwoutH
Copy link
Member Author

EwoutH commented Mar 21, 2023

We didn't have SciPy as an dependency before, so I don't know if that's an acceptable addition.

@Tortar
Copy link
Contributor

Tortar commented Mar 23, 2023

I actually evaluated the possibility to use a KDTree sometime ago instead of the current implementation, unfortunately if I recall well they are really really slower to build and update, in this case this is not showing because flockers has a fixed number of agents so this doesn't apply, but a lot of performance degradation will happen if removal and adding of agents happened. As a side note, I think that you didn't update the KDTree when agents are added, this would result in wrong calculations on the neighbors. But anyway this algorithm is not a good choice for the reasons mentioned above.

@EwoutH
Copy link
Member Author

EwoutH commented Mar 28, 2023

Thanks for this insights, unfortunate to hear but good to know.

I will close this PR.

@EwoutH EwoutH closed this Mar 28, 2023
@wang-boyu wang-boyu mentioned this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants