Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update and add new support for configurations with dynamic Greenland ice sheet component #6226
Update and add new support for configurations with dynamic Greenland ice sheet component #6226
Changes from 21 commits
ef3511b
30f2dd4
6d9af83
0b6899c
31a4bf2
77e453f
9df499b
f38128b
c5a39d1
ed14c21
18965ab
7f2e5a6
71ff4fb
dd43257
7e0da04
423b9a3
0179780
202f394
e6d689d
88d2dc1
6a39312
7d8afb5
7077dbd
28b66fd
c0abc0b
1327c10
057cc51
133dada
aca8a93
3306bae
d8e3c71
4d2427b
b11528d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
see note below - do we really want to add both this and the R2 version?
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.
I will leave this question open for now (see initial response to this above and comment further there).
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.
Should we put these changes in the gnu environment_variables compiler like done in Chrysalis?
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.
Take this out of e3sm_developer for now since it has to run with gnu.
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.
Done. Thanks.
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.
What is the
everywhere
in this file name referring to? I don't think we'd ever want glacier land units to be active everywhere on the LND grid if that's what it means - I think that would have a noticeable degradation on ELM performance.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.
It is what it sounds like - the mask is true (ones) "everywhere". Since we're really only using the 20km GIS grid for testing and debugging, I figured this was ok (I've not noted any performance hit -- it's usually being run on only 1 or 2 nodes -- but it's certainly possible there is one). I made a few other attempts to "fix" the existing mask that was resulting in runtime errors but was unsuccessful. We can certainly do better if this seems important.
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.
I see - I wasn't aware the existing one was causing an error. Activating glacier land units for the low res testing mesh seems fine.
Did you try it with
lnd/clm2/griddata/glcmaskdata_0.5x0.5_GIS_AIS.nc
? My guess is the error comes when the GLC mesh extends beyond the masked region on the LND grid, which could be the case for the 20km GIS mesh. I'm asking because down the road we may need to make our GIS meshes extend farther to ensure they have overlap with the OCN meshes, so if there is an extent issue with the 20km GIS mesh, it will likely show up then too. (not necessary to figure out for this 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.
I don't think we want to be changing this in this PR. @stephenprice , on the call today, Chloe described a new PR she will make that includes this plus a few firn parameter tuning improvements that were used in the long firn spinups.
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.
That's fine, but a few weeks ago she asked me to just include this here. I'm happy to remove it.
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.
@chloewhicker recently opened a PR that includes this change with a few other changes for the firn model in #6298, so I think it should be removed from this 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.
Sorry @stephenprice I didn't know I needed to submit the firn model changes when we talked about it. Matt is right, this line is included in my PR here: #6298 (review)
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.
Ok. Not sure I need to revert this commit though as it will resolve itself in the merge, right?
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.
#6298 has been merged already, so I think it is safe to leave this change here. I looked a the git history, and this change was part of a commit with changes we want to keep, so dropping this change is not as simple as just removing a commit from the branch history. I'm ok with leaving it in, unless @jonbob thinks it would be cleaner to remove it from the history or change it back.
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.
If it only affects cases with an active ice sheet model, its fine.