-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fix mpas mesh tools #450
Fix mpas mesh tools #450
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! This is a great thing to fix. I have a few suggestions that we can discuss when we meet next, or here if you prefer.
mesh_tools/planar_grid_transformations/set_lat_lon_fields_in_planar_grid.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mesh_tools/planar_grid_transformations/set_lat_lon_fields_in_planar_grid.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the two super nit-picky comments I just put in, this looks great! I just want to run a quick test, then I'll approve.
Hmm, my tests weren't successful. I did this to create a test conda environment:
I verified that it worked (that it has the expected entry point):
But when I ran it on a file we use in COMPASS, it didn't work:
It needs to be With this fix, I was able to generate a script file that was identical to the one generated by the old Similarly, when I tried to run
This time, When I made this change, I was able to add latitude and longitude to the mesh for the compass dome test case:
Things looked as expected, though this wasn't a very rigorous test because longitudes are all between 0 and pi/2... |
@hollyhan, I retested tested with the changes you made. I had to go to a little extra trouble to get the C++ tools without actually building the conda package:
That creates an environment with the latest release of Then I install over top of it with the changes you made:
This is a little risky in general for reasons I'm happy to explain (dependencies may have changed since the last release, for example) but it's good enough for testing. Then, I created a mesh to test with:
This all worked great. Then, I tested the SCRIP file generation:
This was a surprise because I thought this worked when I tested yesterday. But it seems like we're missing an import here:
I added this myself and things worked as expected. |
Thanks for going through the testing and explaining the steps in detail, @xylar ! Please let me know if there's anything I should do further to get this PR closed. |
@hollyhan, as I said above, Then, you and I need to work together on rebasing this branch to clean thing up into just 2 or 3 commits. |
@hollyhan, would you be willing to change Then, I'll work with you to rebase the PR into 2 or 3 commits as I mentioned last week. |
@xylar Yes, I'll work on the check for the range of lonCell and lonVertex before we meet on Wednesday. |
Add edits such that resulting latitude field uses positive longitude convention [0 2pi]
Also fix numpy import typo
97c6f6f
to
5cfd4a9
Compare
@hollyhan, so we still missed a step in
Could you add the missing import:
and do the following?
|
- Expected longitude range for the MPAS scripfiles is [0 2pi] - Also import constants from mpas_tools.cime
5cfd4a9
to
fe93509
Compare
@xylar I've added the missing import and pushed the branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, @hollyhan, that did the trick.
I have tested this to my satisfaction. Thanks so much for the hard work. It seemed like a simple task but I managed to make it complicated!
@xylar, I tested the new
|
@xylar and I cleaned up the previous commits by git-rebasing and made new changes to the code - the code now checks for the proper range [0 2pi] of longitude for MPAS scripfiles and raises a value error if the range is outside of [0 2pi] rather than changing the longitude convention to [0 2pi]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hollyhan , these changes make sense based on where the discussion ended up. If you haven't already done so, it's probably a good idea to rerun your workflow of adding lat/lon to a mesh file, running the remap tool, and making sure the map file works properly for regridding inside your SLM code in MALI.
@hollyhan, I'll merge this as soon as you have re-tested your workflow and let us know that it worked as expected. |
Hi @xylar and @matthewhoffman, I have tested the entire workflow and and confirmed that the new workflow passes the test. The following is how I've tested: 1. Try creating a scripfile from a thwaites mesh that uses undesired longitude convention:
Error appears as expected. 2. Change the longitude convention with the new changes made in this PR
3. Create a scripfile with the new mesh that uses the desired longitude convention
4. Create a mapfile going from the mpas mesh to the gaussian grid using ESMF weight generator.
5. Interpolate ice thickness from the thwaites mesh to the global gaussian grid using ncremap
The resulting interpolated thwaites ice thickness is correct. |
@hollyhan, thanks for documenting your testing carefully. It looks good! |
Yes, thanks @hollyhan for documenting the workflow! I'm sure it will be useful to have this for reference in the PR history. |
@matthewhoffman, @hollyhan and @trhille, I'll do a release of MPAS-Tools soon and update the version in |
Hi Xylar and Matt,
In discussing with @matthewhoffman, we found the need to make some changes to two mesh tool scripts so that the mesh/scripfiles generated from these tools have positive longitude convention [0 2pi], which is required by MPAS mesh spec (https://mpas-dev.github.io/files/documents/MPAS-MeshSpec.pdf)
Your review on these changes would be very appreciated.
Thank you!
Holly