jigsaw_to_netcdf: bug fix for POINT coordinates #549

cwsmith · 2024-01-25T21:11:30Z

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: [email protected]
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!

see https://github.com/dengwirda/jigsaw/wiki/MSH-File-Format#point-segment

xylar

A small suggestion. If we're not assuming the mesh dimension has to be 3, let's be explicit about the fact that we assume it's either 2 or 3.

conda_package/mpas_tools/mesh/creation/jigsaw_to_netcdf.py

Co-authored-by: Xylar Asay-Davis <[email protected]>

xylar · 2024-01-29T17:53:13Z

@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 · 2024-01-29T17:55:32Z

@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 · 2024-01-29T19:00:28Z

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 · 2024-01-29T20:01:17Z

I'm actually seeing failures during landice mesh creation due to this recent merge: #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 · 2024-01-29T20:13:53Z

@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 · 2024-01-29T20:46:03Z

@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 · 2024-01-29T20:49:49Z

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

trhille · 2024-01-31T04:09:37Z

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

xylar · 2024-01-31T09:12:00Z

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

matthewhoffman · 2024-02-01T03:43:51Z

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 #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.

trhille

Thanks @cwsmith! I merged this into my local MPAS-Tools/master, installed it in my compass dev using python -m pip install -e conda_package, and ran all the landice mesh generation test cases with this fix. All tests passed. I have one small comment, but I'll let you decide whether to implement it.

trhille · 2024-02-01T23:32:09Z

conda_package/mpas_tools/mesh/creation/jigsaw_to_netcdf.py

+    elif msh['NDIMS'] == 3:
+        zCell_full = msh['POINT'][:, 2]
+    else:
+        raise ValueError(f'Unexpected mesh NDIMS: {msh["NDIMS"]}')


It might be helpful to specify in the error message that NDIMS should be 2 or 3.

Good suggestion, @trhille. That's my fault for suggesting a relatively unhelpful error message.

Good catch. da0d533 has the verbose error message.

xylar · 2024-02-02T17:02:00Z

Okay, merging as soon as CI passes...

jig-to-nc: 3rd value for POINT not coord in 2d

5758643

see https://github.com/dengwirda/jigsaw/wiki/MSH-File-Format#point-segment

cwsmith changed the title ~~jig-to-nc: 3rd value for POINT not coord in 2d~~ jigsaw_to_netcdf: bug fix for POINT coordinates Jan 25, 2024

cwsmith marked this pull request as ready for review January 25, 2024 23:00

xylar reviewed Jan 26, 2024

View reviewed changes

conda_package/mpas_tools/mesh/creation/jigsaw_to_netcdf.py Outdated Show resolved Hide resolved

jigsaw_to_netcdf: only support 2d and 3d meshes

9480b33

Co-authored-by: Xylar Asay-Davis <[email protected]>

xylar mentioned this pull request Jan 29, 2024

setup: always copy subdirs ocean, landice,... #550

Merged

xylar closed this in #550 Jan 29, 2024

xylar reopened this Jan 29, 2024

xylar added bug conda package labels Jan 29, 2024

xylar approved these changes Jan 29, 2024

View reviewed changes

xylar self-assigned this Jan 29, 2024

xylar requested review from matthewhoffman and trhille January 29, 2024 17:52

matthewhoffman approved these changes Feb 1, 2024

View reviewed changes

trhille approved these changes Feb 1, 2024

View reviewed changes

jigsaw_to_netcdf: more helpful error message

da0d533

xylar merged commit 2316a63 into MPAS-Dev:master Feb 2, 2024
7 checks passed

cwsmith deleted the cws/jigToNcPoints2D branch February 2, 2024 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jigsaw_to_netcdf: bug fix for POINT coordinates #549

jigsaw_to_netcdf: bug fix for POINT coordinates #549

cwsmith commented Jan 25, 2024 •

edited

Loading

xylar left a comment

xylar commented Jan 29, 2024

xylar commented Jan 29, 2024

cwsmith commented Jan 29, 2024

trhille commented Jan 29, 2024

cwsmith commented Jan 29, 2024 •

edited

Loading

cwsmith commented Jan 29, 2024

trhille commented Jan 29, 2024

trhille commented Jan 31, 2024

xylar commented Jan 31, 2024

matthewhoffman commented Feb 1, 2024

trhille left a comment •

edited

Loading

trhille Feb 1, 2024

xylar Feb 2, 2024

cwsmith Feb 2, 2024

xylar commented Feb 2, 2024

jigsaw_to_netcdf: bug fix for POINT coordinates #549

jigsaw_to_netcdf: bug fix for POINT coordinates #549

Conversation

cwsmith commented Jan 25, 2024 • edited Loading

details

compass test setup

env sanity check

run compass tests

xylar left a comment

Choose a reason for hiding this comment

xylar commented Jan 29, 2024

xylar commented Jan 29, 2024

cwsmith commented Jan 29, 2024

trhille commented Jan 29, 2024

cwsmith commented Jan 29, 2024 • edited Loading

cwsmith commented Jan 29, 2024

trhille commented Jan 29, 2024

trhille commented Jan 31, 2024

xylar commented Jan 31, 2024

matthewhoffman commented Feb 1, 2024

trhille left a comment • edited Loading

Choose a reason for hiding this comment

trhille Feb 1, 2024

Choose a reason for hiding this comment

xylar Feb 2, 2024

Choose a reason for hiding this comment

cwsmith Feb 2, 2024

Choose a reason for hiding this comment

xylar commented Feb 2, 2024

cwsmith commented Jan 25, 2024 •

edited

Loading

cwsmith commented Jan 29, 2024 •

edited

Loading

trhille left a comment •

edited

Loading