MPAS-Dev / MPAS-Tools

MPAS Tools Repository
Other
38 stars 64 forks source link

jigsaw_to_netcdf: bug fix for POINT coordinates #549

Closed cwsmith closed 8 months ago

cwsmith commented 8 months ago

This PR fixes a bug where the third column of the POINTS data from a Jigsaw file is read as a spatial coordinate for 2D when it should be read as an ID, or at least ignored. The change in this PR simply ignores it for 2D meshes. I may submit a follow up PR to extract the ID for ongoing landice work.

The following Jigsaw wiki describes the mesh file format: https://github.com/dengwirda/jigsaw/wiki/MSH-File-Format#point-segment

Running jigsaw_to_netcdf and MpasMeshConverter.x, with Jigsaw 1.0.0, on a 2d EUCLIDEAN-MESH mesh (gis_2d_pointIds.zip) will demonstrate the bug with master. Should I add this mesh to a test data set?

All the compass mesh_gen tests are passing with this change. The testing setup and test output is below. As a sanity check I've also included the output of pip show mpas_tools that indicates that my local mpas_tools is used within the testing environment created with compass setup.

details

compass test setup

(cmd)(mpasMain) cwsmith@checkers: /space/cwsmith/compassLandice/main (main)$ compass setup -w /space/cwsmith/compassLandice/mesh_gen_compass_main -f checkers.cfg -n 0 43 44 92 93 99
Setting up test cases:
  landice/antarctica/mesh_gen
  landice/greenland/mesh_gen
  landice/humboldt/mesh_gen
Downloading humboldt_1km_2020_04_20.epsg3413.icesheetonly.nc (42.5MiB)
  to /space/cwsmith/compassLandice/main/mpas-albany-landice
100% |#########################################################################################################################################################################################################################################################| Time:  0:00:02
  humboldt_1km_2020_04_20.epsg3413.icesheetonly.nc done.
  landice/kangerlussuaq/mesh_gen
Downloading greenland_8km_2020_04_20.epsg3413.nc (4.3MiB)
  to /space/cwsmith/compassLandice/main/mpas-albany-landice
100% |#########################################################################################################################################################################################################################################################| Time:  0:00:00
  greenland_8km_2020_04_20.epsg3413.nc done.
  landice/koge_bugt_s/mesh_gen
  landice/thwaites/mesh_gen
Downloading antarctica_1km_2020_10_20_ASE.nc (559.3MiB)
  to /space/cwsmith/compassLandice/main/mpas-albany-landice
100% |#########################################################################################################################################################################################################################################################| Time:  0:00:26
  antarctica_1km_2020_10_20_ASE.nc done.
target cores: 128
minimum cores: 1

env sanity check

(ins)(mpasMain) cwsmith@checkers: /space/cwsmith/compassLandice/mesh_gen_compass_main $ pip  show mpas_tools
Name: mpas_tools
Version: 0.30.0
Summary: A set of tools for creating and manipulating meshes for the climate components based on the Model for Prediction Across Scales (MPAS) framework
Home-page: https://github.com/MPAS-Dev/MPAS-Tools
Author: MPAS-Tools Developers
Author-email: mpas-developers@googlegroups.com
License: BSD
Location: /space/cwsmith/compassLandice/MPAS-Tools/conda_package
Requires: cartopy, cmocean, dask, igraph, inpoly, matplotlib, netcdf4, numpy, progressbar2, pyamg, pyevtk, pyproj, scikit-image, scipy, shapely, xarray
Required-by: 

run compass tests

(ins)(mpasMain) cwsmith@checkers: /space/cwsmith/compassLandice/mesh_gen_compass_main $ compass run
landice/antarctica/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        03:12
landice/greenland/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        03:25
landice/humboldt/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        03:02
landice/kangerlussuaq/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        00:11
landice/koge_bugt_s/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        00:14
landice/thwaites/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        01:24
Test Runtimes:
03:12 PASS landice_antarctica_mesh_gen
03:25 PASS landice_greenland_mesh_gen
03:02 PASS landice_humboldt_mesh_gen
00:11 PASS landice_kangerlussuaq_mesh_gen
00:14 PASS landice_koge_bugt_s_mesh_gen
01:24 PASS landice_thwaites_mesh_gen
Total runtime 11:29
PASS: All passed successfully!
xylar commented 8 months ago

@matthewhoffman and/or @trhille, is this a script you currently use? If so, could you try it out and make sure @cwsmith's changes fix issues and/or don't break anything in your current workflows?

xylar commented 8 months ago

@matthewhoffman and @trhille, sorry, I hadn't read @cwsmith's description above carefully. It would appear you all need this fix for your workflows to work with the Jigsaw version we are now using in Compass. So it would be relatively urgent to get this reviewed and merged (and Compass updated to use a new version of MPAS-Tools with this fix).

cwsmith commented 8 months ago

At least for the Greenland and Antarctica compass landice mesh gen test cases, the fix may not be required as the ID column/entry in the Jigsaw mesh file/data is always zero which is the coordinate that those cases need.

trhille commented 8 months ago

I'm actually seeing failures during landice mesh creation due to this recent merge: https://github.com/MPAS-Dev/MPAS-Tools/pull/543. That's not a big deal in itself, although it shows that we need to update the data files on the LCRC server. But it makes me think I'm testing this incorrectly. I activated my compass env, checked out this MPAS-Tools branch, and then executed python -m pip install -e conda_package before setting up and running the mesh_gen test cases. @xylar, is that the right way to go about it? @cwsmith, is that what you did?

cwsmith commented 8 months ago

@trhille Yeah, I use that same process to setup a dev branch of MPAS-Tools.

I'm not sure why I didn't see that error. My only guess is that maybe the modified script from 543 wasn't copied into my previously existing 'install' dir. I can try to reproduce what you are seeing if that would be helpful.

cwsmith commented 8 months ago

@trhille I rebased my dev branch on master and am hitting an assertion associated with the basal heat flux.

AssertionError: basalHeatFlux contains negative values! This is likely due to the conventions used in the input file, rather than bad data. Ensure non-negative values before interpolating.

Is that what you saw?

trhille commented 8 months ago

@cwsmith, yes that's the error I'm getting. I'll discuss with @matthewhoffman about the best way to fix this.

trhille commented 8 months ago

@xylar and @cwsmith, I have two PRs open that will fix the issue with above landice mesh generation: https://github.com/MPAS-Dev/MPAS-Tools/pull/551 and https://github.com/MPAS-Dev/compass/pull/767. We'll try to get those merged asap so that this can move ahead.

xylar commented 8 months ago

@trhille, it looks like you don't need me to review either PR but let me know if you need any help testing.

matthewhoffman commented 8 months ago

I tested this by checking out compass origin/main: * 84d8882fe (HEAD, origin/main, origin/HEAD) Merge pull request #767 from trhille/landice/update_lcrc_files and using python -m pip install -e . to set MPAS-Tools in my conda env to the commit before https://github.com/MPAS-Dev/MPAS-Tools/pull/551: * 5d77fca8 Merge pull request #550 from SCOREC/cws/copyScripts I expect the Thwaites mesh_gen case to die with the bheatflx assertion error because of NaNs in the source file, and it does:

## basalHeatFlux ##
    ---- Interpolating time level 0 ----
  Input field  bheatflx min/max: nan nan
  ...Interpolating to basalHeatFlux using built-in bilinear method...
  interpolated MPAS basalHeatFlux min/max: nan nan
  interpolation done in 0.20962685999984387

/global/cfs/cdirs/fanssie/users/hoffman2/mambaforge/envs/dev_compass_1.2.0-alpha.9/bin/interpolate_to_mpasli_grid.py:4: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  __import__('pkg_resources').require('mpas-tools==0.30.0')
Traceback (most recent call last):
  File "/global/cfs/cdirs/fanssie/users/hoffman2/mambaforge/envs/dev_compass_1.2.0-alpha.9/bin/interpolate_to_mpasli_grid.py", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/global/cfs/cdirs/fanssie/users/hoffman2/MPAS-Tools/conda_package/landice/mesh_tools_li/interpolate_to_mpasli_grid.py", line 777, in <module>
    assert MPASfield.min() >= 0.0, 'basalHeatFlux contains negative values! This is likely due to the ' \
AssertionError: basalHeatFlux contains negative values! This is likely due to the conventions used in the input file, rather than bad data. Ensure non-negative values before interpolating.

      Failed

I then update MPAS-Tools to the current head: * 083600a6 (origin/master, origin/HEAD) Merge pull request #551 from trhille/landice/fix_assert_positive_basalHeatFlux and then merge in this PR (549). I update MPAS-Tools in my conda env with python -m pip install -e . and rerun the test. Now the test passes, indicating these two MPAS-Tools PRs are working together as intended.

  test execution:      SUCCESS
  test runtime:        06:36
Test Runtimes:
06:36 PASS landice_thwaites_mesh_gen
Total runtime 06:49
PASS: All passed successfully!

I'll wait to confirm that everyone thinks this is adequate testing before I merge it. Thanks @cwsmith and @trhille for sorting out this complicated situation.

xylar commented 8 months ago

Okay, merging as soon as CI passes...