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

Two libraries used for KDTree in Parcels. Use only one? #1616

Closed
erikvansebille opened this issue Jul 26, 2024 · 2 comments · Fixed by #1666
Closed

Two libraries used for KDTree in Parcels. Use only one? #1616

erikvansebille opened this issue Jul 26, 2024 · 2 comments · Fixed by #1666
Assignees
Labels
cleanup Cleaning up legacy code coding/Python good first issue Good for new parcels developers

Comments

@erikvansebille
Copy link
Member

Throughout the codebase, Parcels uses two different libraries for kdtree:

https://github.com/OceanParcels/parcels/blob/035159a1b4002bc1550cebd275f93c0adaa8513d/parcels/particleset.py#L18-L21

https://github.com/OceanParcels/parcels/blob/035159a1b4002bc1550cebd275f93c0adaa8513d/parcels/interaction/neighborsearch/kdtreeflat.py#L2

Perhaps we should standardise this and use only one of them? Since scipy is already a dependency, perhaps better to use scipy.spatial.KDTree throughout?

@erikvansebille erikvansebille added good first issue Good for new parcels developers coding/Python labels Jul 26, 2024
@VeckoTheGecko VeckoTheGecko added the cleanup Cleaning up legacy code label Jul 26, 2024
@VeckoTheGecko
Copy link
Contributor

Note that tutorial_nemo_curvilinear.ipynb should be updated to remove the comment "Note that this only works if you have the pykdtree package installed, which is only included in the Parcels dependencies in version >= 2.2.2"

@VeckoTheGecko
Copy link
Contributor

Looking into this a bit more, pykdtree is still an active project. I think this is due to their benchmarking being better than the SciPy implemnetation, though I doubt these benchmarks are accurate since they're 12 years old (and I imagine scipy's performance may have significantly improved).

Let's go ahead with the removal, and if updated benchmarks show pykdtree is significantly better we can consider re-introducing (especially since the part of the code calling it looks to be part of the chunking algorithm).

Related: storpipfugl/pykdtree#121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleaning up legacy code coding/Python good first issue Good for new parcels developers
Projects
Archived in project
2 participants