-
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
New floating land ice variables #512
Conversation
@xylar This is for you to peruse. |
@cbegeman, I think I might need to fix something in the |
@xylar That seems reasonable. The renaming here led me to believe that all
|
Yes, you're right. But with the thin film case, we need those to be distinct concepts and there's just not the right hooks for that right now. So I'll maybe fiddle around a bit and add a commit to this branch if you don't mind. |
@xylar Sounds good. Thanks! |
51ad833
to
d5cba56
Compare
Testing |
d5cba56
to
2e403c3
Compare
Noting that I have set the thin film cells to the local freezing point based on |
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.
@cbegeman, now that the E3SM PR has been merged, this will need a rebase and for the submodule to point to the merge commit for E3SM-Project/E3SM#5464 (see comment below). I tried to do the rebase myself but I was unsure about some of the changes. If you want, we can talk it over at our meeting today and I can take care of it after that.
099d59c
to
6373013
Compare
6373013
to
f904806
Compare
f904806
to
e1fb0b9
Compare
I need to run the test cases with ice-shelf cavities and cache new initial conditions. The |
@cbegeman, are baseline comparison failures to be expected for
|
The plots look fine... |
Oh, no! I'm seeing this error in init mode, too:
I believe this is happening before we have a chance so set values for |
Yeah, we don't want this: |
I'll make a bugfix PR for this shortly. |
As we discussed on Slack, this was my mistake. I tried to save myself some trouble with caching by using a cached |
With the new cache files, the So as far as I'm concerned, the only loose end is ISOMIP+. |
This makes it possible to treat the thin-film and non-thin-film cases more similarly when in comes to masking and interpolating topography and fractional variables as long as the ocean fraction is handled appropriately (either including or exclusing grounded ice).
These need to include the new `landIceFloating*` fields.
2a38bb4
to
221e20f
Compare
@xylar I pushed two bugfixes that much reduce the diffs, though not to 0 because there are intentional differences in the |
@cbegeman, okay, that's fine. I knew there might be differences but I was a little concerned they were larger than expected. Let me test again. |
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.
@cbegeman, yep the differences look reasonable to me. Thanks for working so hard on this!
@xylar Thanks for your review! |
This PR adds new variables to represent floating land ice in a manner that is consistent with E3SM-Ocean-Discussion/E3SM#38
Checklist
E3SM-Project
submodule has been updated with relevant E3SM changesTesting
in this PR) any testing that was used to verify the changes