MPAS-Dev / MPAS-Tools

MPAS Tools Repository
Other
37 stars 63 forks source link

Switch from `pyflann` to scipy `KDTree` #442

Closed xylar closed 2 years ago

xylar commented 2 years ago

Anywhere there was the option to use both, there is now only one option (always 'kdtree').

This merge also updates ci builds of the conda package to use libnetcdf 4.8.1.

closes #441

xylar commented 2 years ago

Testing

I ran the nightly compass ocean test suite using a test build of this package on my Ubuntu laptop with gnu compilers and both mpich and openmpi. It was bit-for-bit for all tests that ran (tests with more than 4 cores fail). This isn't a terribly rigorous test because these tests don't make much use of the KDTree.

I also created the 4 largest global meshes with this branch and compared against the same using the current release of mpas_tools. As expected, the meshes that use signed distance functions end up with different numbers of cells and can't be compared directly with one another. However, the meshes were constructed in nearly identical time, suggesting no performance hit from this change.

Old mpas_tools:

10:23 PASS ocean_global_ocean_EC30to60_mesh
10:36 PASS ocean_global_ocean_ECwISC30to60_mesh
29:52 PASS ocean_global_ocean_SOwISC12to60_mesh
23:37 PASS ocean_global_ocean_WC14_mesh

This branch:

10:21 PASS ocean_global_ocean_EC30to60_mesh
10:37 PASS ocean_global_ocean_ECwISC30to60_mesh
29:53 FAIL ocean_global_ocean_SOwISC12to60_mesh
23:48 FAIL ocean_global_ocean_WC14_mesh

Fails are in baseline comparison because of different numbers of cells.

xylar commented 2 years ago

@sbrus89, do you have a plausible way to run any tests with this branch? Would you need me to make a test build of the package and/or a development environment to test with?

@dengwirda, let me know if you would like to test this. If not, that's fine. I would still appreciate having you take a quick look at the code, particularly to make sure it doesn't make it harder for you to add support for geodesic distance in the future.

xylar commented 2 years ago

@dengwirda, I just added support for workers with the default being workers=-1. Let me know how it looks. I'll test tomorrow but I'm off to bed for now.

sbrus89 commented 2 years ago

@sbrus89, do you have a plausible way to run any tests with this branch? Would you need me to make a test build of the package and/or a development environment to test with?

There are tests in legacy compass that use the costal_tools.py script. If you could create a developer environment, that would be great. Then I can run those tests.

xylar commented 2 years ago

If you could create a developer environment, that would be great. Then I can run those tests.

@sbrus89, sure. What machine would you like to use? Chrysalis?

sbrus89 commented 2 years ago

@xylar, Compy would be great, if it's all the same to you. Otherwise Chrysalis is good too.

xylar commented 2 years ago

Compy it is! Not my preferred machine ;-) But it's no harder for me to set things up there than anywhere else.

xylar commented 2 years ago

@sbrus89, Compy is very slow but soon you should be able to run:

source /share/apps/E3SM/conda_envs/base/etc/profile.d/conda.sh
conda activate test_compass_0.3.0

And you should have the test environment you need. As far as compiling and running MPAS-Ocean for legacy, you can try the following, which is based on the load script for the new compass:

source /share/apps/E3SM/conda_envs/base/etc/profile.d/conda.sh
conda activate test_compass_0.3.0

module purge
module load cmake/3.19.6
module load gcc/8.1.0
module load intel/19.0.5
module load intelmpi/2019u4
module load netcdf/4.6.3
module load pnetcdf/1.9.0
module load mkl/2019u5

export NETCDF=$(dirname $(dirname $(which nc-config)))
export NETCDFF=$(dirname $(dirname $(which nf-config)))
export PNETCDF=$(dirname $(dirname $(which pnetcdf-config)))

export PIO=/share/apps/E3SM/conda_envs/compass/system/compass_1.0.0/intel/impi/scorpio_1.2.1
export USE_PIO2=true
export HDF5_USE_FILE_LOCKING=FALSE
xylar commented 2 years ago

@dengwirda, would you want to do any further testing of this change? If not, I'd like to get it in an make a new release relatively soon so the mpas_tools package can be updated to the latest hdf5.

xylar commented 2 years ago

Thanks @dengwirda and @sbrus89!