-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add a circular ice sheet test for the coupled MALI-Sea level model #748
base: main
Are you sure you want to change the base?
Add a circular ice sheet test for the coupled MALI-Sea level model #748
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.
@hollyhan and @xylar , I did a quick pass through this PR and don't see any concerns with the overall structure. @hollyhan , this is great work, and I am very happy we have this idealized setup fully automated within compass! Once this is complete, I will think about how to add a version of this to our full_integration suite to be sure we are testing SLM with every PR.
I added a number of comments as I went - I'm sure you are aware of many of these clean up items, but I figured I might as well flag them as I went. There are a few slightly more significant items I mention - feel free if you want to discuss any of them. And like we talked about, for anything that seems beyond what you actually care about, let me know and I am happy to contribute. Xylar and I can do a more thorough review once the docs are ready and any other details are finalized.
|
||
# other constants used in the fuction | ||
# pi = 3.1415926535898 # pi value used in the SLM | ||
rhoi = 910.0 # ice density |
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.
May want to get this from namelist or cfg to be safe. Could be added later
smbfile.close() | ||
|
||
|
||
def _build_mapping_files(config, logger, res, nglv, mali_mesh_file): |
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.
Is this identical (or very similar) to the version in the ISMIP6-2300 SLM test case PR (#749)? If so, we should move this to a compass landice framework module and call it from both places. That does not need to need to be in the scope of your revisions - I can do that in a follow up PR.
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.
Sounds good - I'll leave this comment unresolved so you can follow up with it.
# adjust the lat-lon values | ||
args = ['set_lat_lon_fields_in_planar_grid.py', | ||
'--file', mali_mesh_file, | ||
'--proj', 'ais-bedmap2-sphere'] |
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.
Flagging again that we need to add this in an MPAS-Tools PR. (Already mentioned in #749)
@hollyhan and @matthewhoffman, I just wanted to check in on this one. What's the status? |
This is largely copied from the dome smoke test and has stretches of code from the test left in place as an example.
for the ice mass change variables
MPE: mean percentage error SLC: sea-level change Also enable the SLM output class to take in an assigned number of time indices of outputs to be analyzied
77d6c8a
to
27ad364
Compare
Checklist
api.rst
) has any new or modified class, method and/or functions listedE3SM-Project
submodule has been updated with relevant E3SM changesMALI-Dev
submodule has been updated with relevant MALI changesTesting
in this PR) any testing that was used to verify the changes