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

Landice interpolation unification #817

Merged
merged 13 commits into from
Oct 2, 2024

Conversation

andrewdnolan
Copy link
Contributor

@andrewdnolan andrewdnolan commented Apr 16, 2024

With PRs #803 and #750 the mesh_gen testcases in the greenland and antarctica test groups now make better use of mesh generation utilities within the landice framework (e.g. compass.landice.mesh). But there is still some redundant code as it relates to interpolation (e.g. interp_ais_bedmachine and interp_ais_bedmachine could be generalized to interp_ais). Continuing along that line, there is no real need to have separate interpolation functions for Antarctica (interp_ais) and Greenland (interp_gis).

The purpose of the PR is generalize the existing interpolation functions to be applicable the standard datasets (i.e. measures and bedmachine) and standard icesheets (Antarctica and Greenland). With the generalized interpolation routine, the respective mesh_gen testcases are cleaned up and unified as much as possible.

Checklist

  • User's Guide has been updated
  • Developer's Guide has been updated
  • API documentation in the Developer's Guide (api.rst) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Document (in a comment titled Testing in this PR) any testing that was used to verify the changes

@andrewdnolan andrewdnolan added land ice in progress This PR is not ready for review or merging labels Apr 16, 2024
@andrewdnolan
Copy link
Contributor Author

Testing

  • Machine: perlmutter
  • Compass version: 1.3.0-alpha.2

Command Run :

compass setup -n 0 45 -w wrkdir

Results

landice/antarctica/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        37:42
landice/greenland/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  test runtime:        15:36
Test Runtimes:
37:42 PASS landice_antarctica_mesh_gen
15:36 PASS landice_greenland_mesh_gen
Total runtime 53:30
PASS: All passed successfully!

@andrewdnolan andrewdnolan marked this pull request as ready for review April 18, 2024 15:47
@andrewdnolan andrewdnolan force-pushed the landice/interpolation_unification branch from 32bb814 to f50b00d Compare May 23, 2024 17:08
@trhille
Copy link
Collaborator

trhille commented Aug 15, 2024

CI seems to be broken. I'm going to close and reopen to see if that jump-starts it.

@trhille trhille closed this Aug 15, 2024
@trhille trhille reopened this Aug 15, 2024
@trhille trhille force-pushed the landice/interpolation_unification branch 2 times, most recently from 0971531 to 21afbfd Compare August 15, 2024 20:32
@trhille
Copy link
Collaborator

trhille commented Aug 16, 2024

@andrewdnolan, the landice/antarctica/mesh_gen test passes execution for me. It fails baseline comparison with the current head of compass/main. That's not necessarily a problem, but we should look into it.
landice/greenland/mesh_gen fails execution however. Here's the traceback:

Setting cell_width in outer regions to max_spac for 654997 cells
calling preprocess_gridded_data
      Failed
Exception raised while running the steps of the test case
Traceback (most recent call last):
  File "/global/cfs/cdirs/fanssie/users/trhille/compass/compass/run/serial.py", line 322, in _log_and_run_test
    _run_test(test_case, available_resources)
  File "/global/cfs/cdirs/fanssie/users/trhille/compass/compass/run/serial.py", line 419, in _run_test
    _run_step(test_case, step, test_case.new_step_log_file,
  File "/global/cfs/cdirs/fanssie/users/trhille/compass/compass/run/serial.py", line 470, in _run_step
    step.run()
  File "/global/cfs/cdirs/fanssie/users/trhille/compass/compass/landice/tests/greenland/mesh.py", line 89, in run
    preprocessed_gridded_dataset = preprocess_gridded_data(
                                   ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/global/cfs/cdirs/fanssie/users/trhille/compass/compass/landice/mesh.py", line 912, in preprocess_gridded_data
    gg.variables['thk'][0, :, :] *= floodFillMask
  File "/pscratch/sd/t/trhille/miniforge3/envs/dev_compass_1.4.0-alpha.7/lib/python3.11/site-packages/numpy/ma/core.py", line 4415, in __imul__
    self._data.__imul__(other_data)
ValueError: operands could not be broadcast together with shapes (2816,1664) (1408,832) (2816,1664) 

Any idea what might be causing this?

Added a single function that should incorporate all the functionality
of the exisiting `compass.landice.mesh.interp_ais_bedmachine()` and
`compass.landice.mesh.interp_ais_measures()` functions, but that is
general enough to work for both GIS and AIS, as well as for MEASURES
and BedMachine datasets.
Add the list of requested variables to the `args` list by concating.
Previously I was manually creating the space seperated list (i.e string)
when a list of variables was passed to the girdded inerpolator function.
This space seperated string was not correctly parsed by the
`mpas_tool.logging.check_call` function resulting in the variables not
being inerpolated.
@andrewdnolan andrewdnolan force-pushed the landice/interpolation_unification branch from 21afbfd to b281644 Compare September 13, 2024 20:49
@andrewdnolan
Copy link
Contributor Author

Testing (Part One)


NOTE: Testing needs to be done in two stages. With the addition of globally sorted cell ID's (4bdc715) there's no easy way difference against the mesh generated on main, because the sort_mesh function "resets" the indexToCellID array. (i.e. For both sorted and unsorted meshes the indexToCellID array is monotonically increasing from 1 to nCells).

So, the baseline against main needs to be done from 626f11a then visual testing can be done from 4bdc715 to confirm the sorting has been implemented.


Tested on perlmutter in /pscratch/sd/a/anolan/landice_interpolation:

# using compass env script form main branch
compass setup -n 0 45 -w main@869dafea8 -f custom.cfg
# using compass env script form landice/interpolation_unification branch
compass setup -n 0 45 -w interpolation_unification@626f11a6a -f custom.cfg -b $PSCRATCH/landice_interpolation/main@869dafea8/

pushd main@869dafea8 && sbatch job_script.custom.sh && popd
pushd interpolation_unification@626f11a6a && sbatch job_script.custom.sh && popd

Which results in (for the PR branch):

landice/antarctica/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  baseline comparison: FAIL
  see: case_outputs/landice_antarctica_mesh_gen.log
  test runtime:        35:26
landice/greenland/mesh_gen
  * step: mesh
  test execution:      SUCCESS
  baseline comparison: FAIL
  see: case_outputs/landice_greenland_mesh_gen.log
  test runtime:        17:08
Test Runtimes:
35:26 FAIL landice_antarctica_mesh_gen
17:08 FAIL landice_greenland_mesh_gen
Total runtime 53:13
FAIL: 2 tests failed, see above.

So both test pass but unfortunately the baseline comparison fails. To investigate things a little further, I've written my own mesh differencing script following compass's baseline approach: https://gist.github.com/andrewdnolan/162339430d602229379c83c5f72200f9

Running that script on the mesh produced from 626f11a results in:
image

@matthewhoffman has OKed these difference offline, but might be worth double checking on this.

@andrewdnolan
Copy link
Contributor Author

Testing (Part 2)

With the addition of the sorting commit, here is the indexToCellID array:
image

Copy link
Collaborator

@trhille trhille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewdnolan, this is looking really good! I have a lot of comments but they are all tiny. I'm running tests now.

@@ -441,7 +443,7 @@ def get_dist_to_edge_and_gl(self, thk, topg, x, y,
elif window_size < min(high_dist, high_dist_bed):
logger.info('WARNING: window_size was set to a value smaller'
' than high_dist and/or high_dist_bed. Resetting'
f' window_size to {max(high_dist, high_dist_bed)},'
f' window_size to {max(high_dist, high_dist_bed)}, '
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inadvertent extra space?

compass/landice/mesh.py Outdated Show resolved Hide resolved
compass/landice/mesh.py Outdated Show resolved Hide resolved
compass/landice/mesh.py Outdated Show resolved Hide resolved
compass/landice/mesh.py Outdated Show resolved Hide resolved
compass/landice/tests/greenland/mesh.py Show resolved Hide resolved
compass/landice/tests/greenland/mesh.py Show resolved Hide resolved
compass/landice/tests/greenland/mesh_gen/mesh_gen.cfg Outdated Show resolved Hide resolved
docs/developers_guide/landice/framework.rst Outdated Show resolved Hide resolved
docs/developers_guide/landice/framework.rst Outdated Show resolved Hide resolved
@trhille
Copy link
Collaborator

trhille commented Sep 30, 2024

@matthewhoffman has OKed these difference offline, but might be worth double checking on this.

@andrewdnolan, would you be able to report the L-inf norm for these diffs as well?

@andrewdnolan
Copy link
Contributor Author

Screenshot 2024-09-30 at 2 38 55 PM

Here are the errors with the L-inf norm added. I'll updated the gist linked above in case you want to run things yourself.

@trhille
Copy link
Collaborator

trhille commented Sep 30, 2024

Excellent, thanks. I'm not worried about these diffs at all.

Sorting the global indicies will improve memory performance of
`cellsOnCell` and such. Additionally, sorting will help with
PIO decompostion and ice-sheet-coupler communication.
@andrewdnolan andrewdnolan force-pushed the landice/interpolation_unification branch from 4bdc715 to 4338977 Compare October 1, 2024 18:52
@andrewdnolan
Copy link
Contributor Author

Testing (Part 3)

Here some plots to confirm the sorting is indeed working correctly, now that it's been placed after all the convert function calls.

Slice zeros of edges on cell:
edgesOnCell_slice0_sorted

Index to cell ID:
indexToEdgeID_sorted

@trhille trhille self-requested a review October 2, 2024 03:41
Copy link
Collaborator

@trhille trhille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, @andrewdnolan! I've confirmed that both test cases pass and that the edge sorting issue is resolved. I visually inspected all the important fields, but am relying on the diffs you provided above in order to bless changes.

@trhille trhille merged commit 1279a40 into MPAS-Dev:main Oct 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress This PR is not ready for review or merging land ice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants