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

Glad's is_in_active_grid is slightly inconsistent with logic elsewhere in CISM #41

Open
billsacks opened this issue Sep 1, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@billsacks
Copy link
Member

This is a spin-off from #39 . I noticed that the conditional that determines the icemask in is_in_active_grid checks usrf > 0. But apparently some code elsewhere in CISM treats points with topg exactly equal to 0 as land. So, for consistency, is_in_active_grid should also consider these points with topg exactly equal to 0 to be land, and thus within the ice mask.

As discussed in #39 , this inconsistency is a problem for conservation, so it should definitely be fixed. However, it only impacts a small number of grid cells. As also discussed in #39 , changing this conditional to usrf >= 0 doesn't work, because ocean grid cells have usrf == 0, so that change led all points in the CISM domain to be considered to be within the ice mask. @whlipscomb suggested a possible change like usrf > 0 .or. topg >= 0, but we want to check to make sure that really is the correct conditional, consistent with what is used elsewhere. Ideally, we would use an existing mask variable that is consistent with what is used elsewhere.

@billsacks billsacks added the bug Something isn't working label Sep 1, 2021
@billsacks
Copy link
Member Author

After changing this, a good sanity check that it is correct is: when running the CISM standard testing (aux_glc tests in the CISM-wrapper test suite), answers should change, but only (initially) in a small number of grid cells: those with topg exactly equal to 0.

One way to confirm that we aren't getting inadvertent changes elsewhere could be: create a temporary input file where grid cells with topg == 0 are replaced with topg = 1. (e.g., ncap2 -s 'where(topg == 0.) topg = 1.;' greenland_4km_epsg3413_c171126.nc greenland_4km_epsg3413_c171126_topgNonZero.nc.) Confirm that this only changes a small number of grid cells in the input file. Then run both the baseline and new code with this modified file (e.g., in test SMS_Ly3.f10_f10_mg37.I1850Clm50SpG.cheyenne_intel), and confirm that they are bit-for-bit. This will demonstrate that any answer changes that arise when using the actual input file come from just this small set of grid cells with topg == 0.

billsacks added a commit to billsacks/cism that referenced this issue Sep 1, 2021
…r checks"

This reverts commit 3b899f4.

This didn't work: it resulted in the ice mask being 1 everywhere.

See ESCOMP#39 (comment)

We'll fix this later (see ESCOMP#41)
@billsacks billsacks mentioned this issue Sep 1, 2021
billsacks added a commit that referenced this issue Sep 1, 2021
Revert PR #40

This didn't work: it resulted in the ice mask being 1 everywhere.

See #39 (comment)

We'll fix this later (see #41)
@whlipscomb
Copy link
Contributor

@billsacks – I like your suggestion with the temporary input file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants