search

MPAS-Dev / MPAS-Tools

MPAS Tools Repository
Other
37 stars 63 forks source link

drop (`py`)`flann` from conda package #441

Closed xylar closed 2 years ago

xylar commented 2 years ago

It seems that the flann package is not being maintained. The last commits were in 2019: https://github.com/flann-lib/flann/commits/master Web site links are broken.

This lack of support is likely to manifest itself in an inability to keep mpas_tools updated with the latest conda-forge libraries, which will negatively affect E3SM-Unified as well. The current problem is with: https://github.com/conda-forge/flann-feedstock/pull/31

I suggest that we figure out how to move to a supported package instead.

xylar commented 2 years ago

@dengwirda and @sbrus89, you are the most likely candidates to help me find a suitable alternative. Any thoughts?

xylar commented 2 years ago

@sbrus89, you had implemented the KDTree from scipy as an alternative: https://github.com/MPAS-Dev/MPAS-Tools/blob/ac85e45a1c128f89fac4e4e226b065a176de3d6c/conda_package/mpas_tools/ocean/coastal_tools.py#L671-L672 But you made flann the default. Do you recall why? Was the performance better?

sbrus89 commented 2 years ago

@xylar, yes, the scipy KDTree is also implemented. I ended up using flann because the performance was so much better.

dengwirda commented 2 years ago

@xylar, I've had reasonable success using spatial.cKDTree (I believe it's a compiled version of KDTree) in a remapping context --- maybe this could be worth a try? It's possible to set up to be threaded, so has scaled okay for me so far.

xylar commented 2 years ago

Thanks to both of you. Here is another conversation that @dengwirda and I had earlier that is also relevant: https://github.com/MPAS-Dev/MPAS-Tools/pull/359

xylar commented 2 years ago

I will work on an branch that moves to spatial.cKDTree and we can see how that goes. It still might be worth having @dengwirda's alternative that uses geodesic distance, but that's not related to the immediate concern.

sbrus89 commented 2 years ago

Seems like spatial.cKDTree is the way to go based on #359. I agree that we should move to geodesic distance, if possible.

xylar commented 2 years ago

From the scipy documentation:

cKDTree is functionally identical to KDTree. Prior to SciPy v1.6.0, cKDTree had better performance and slightly different functionality but now the two names exist only for backward-compatibility reasons. If compatibility with SciPy < 1.6 is not a concern, prefer KDTree.

The current version of scipy on conda-forge is 1.7.1, so I think we should just use KDTree as recommended.

sbrus89 commented 2 years ago

That's good to know.