-
Notifications
You must be signed in to change notification settings - Fork 383
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 maint1.2: add 3D varying GM #3513
Conversation
@jonbob This needs the additional 5 GM flags noted here: |
Thanks @mark-petersen - I'll run the scripts to auto-generate the corresponding bid changes and commit them to your branch |
@mark-petersen - I did a test merge and some testing. I don't think we want that last E3SM PR:
The corresponding changes in the cpl aren't there, and it requires more changes to the ocean driver |
c7bfa64
to
4993141
Compare
@darincomeau, and @matthewhoffman, will melt rates work as expected without #2948? |
@xylar , I don't recall. @jonbob , I don't remember the details of #2948 , but the concerning thing is that without it, if |
I think the larger PR that #2948 was fixing was one of Jeremy's (#2726) that @matthewhoffman and I reworked. We would have to integrate both of them into maint-1.2 to get that functionality |
@mark-petersen - my initial tests pass now, with that last commit removed. My inclination is that we should merge this to maint-1.2 and decide separately about the land_ice_flux_mode, which could come in as a different PR. @matthewhoffman , @xylar - is that OK with you? |
@jonbob, yep, that sounds fine to me. I just want us to make sure that melt fluxes work as expected before we get too far running with maint-1.2. |
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.
Looks good visually and initial testing passes after the last commit was removed
Since it is just us using maint-1.2, I think it is OK to merge this and then make more corrections or additions next week. @jonbob is worthwhile requesting that the software engineering team run the E3SM regression suite on maint-1.2 after this (what they normally do for |
I think it's easier if we run whatever tests we think are important. I can run a suite of test if you think we need to |
Is it too late to add #3516, which was just opened? |
No, never too late! |
@darincomeau - though that should probably be a separate PR into maint-1.2, since it's not at all related to this mpas one |
@mark-petersen - I ran at least some small fully-coupled tests. Since this PR will change answers, we're only really going to be able to pass smoke tests... We could try some ERS tests, just to make sure restarts are still BFB |
That occurred to me, but I saw E3SM master commits in as well. But a separate PR works. |
Passed:
@mark-petersen - it's your PR, so please weigh in on the amount of testing required |
We'll run a longer test after merging |
merged to maint-1.2 |
@mark-petersen - no, I'm ahead of you on that one! Please see #3517 |
@mark-petersen - however, we do need to decide whether or not to include Jeremy's work from PR #2726 and PR #2948 |
This update adds the following PRs from MPAS ocean/develop:
and from E3SM master:
[non-BFB]