-
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
Replace init mode in drying slope test cases #716
Replace init mode in drying slope test cases #716
Conversation
TestingDrying slope tests have been run on chrys with intel, impi. Results are qualitatively similar by visual inspection. Before the domain size was changed, the initial state was also verified to be quantitatively similar |
63430d4
to
7cff7e9
Compare
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'm not super involved with the drying slope test case so I'm mostly leaving the review of the details to @sbrus89. The approach here looks great to me! I'm always in favor of moving away from init mode.
@sbrus89 FYI, I'm going to add coriolis parameter as a config option if you want to hold off reviewing. |
@sbrus89 Ready for your review. Thanks! |
@sbrus89 No problem. I'll ping you once I've rebased and retested. |
Thanks very much! |
64ff904
to
b092afc
Compare
@sbrus89 Ready for your review! |
@sbrus89 Just a reminder to review this when you have the chance |
Thanks @cbegeman, sorry for losing track of this. |
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, this looks great and is a huge improvement over the init mode config. I had a couple minor comments. Also, I did notice in testing that the convergence seems to have been altered:
Let me look into this. The change shouldn't have been this large. I may have forgotten to take into account the (new) domain offset when we evaluate convergence. |
b092afc
to
66f2039
Compare
@sbrus89 Thanks for your thorough review! I have now addressed all your comments. Let me know if you spot anything else. |
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, this looks great. Thanks for the effort on this!
@xylar This is ready to merge. Do I have the go-ahead from you? |
Yep, go for it! |
(You don't need to ask me if the PR is assigned to you, you can just merge. But I appreciate you checking!) |
@xylar Thanks! I wanted to check because I assigned it to myself :) |
Ah, yeah, fair enough. You have my blanket permission to assign yourself and merge when you see fit :-) |
This PR replaces the
mode_init
MPAS-Ocean run in theinitial_state
step with local computations. The initial state is unchanged for the configuration that is currently used in the drying_slope cases. I did not retain all of the configuration options that were previously present inmode_init
, e.g., the idealized transect.I added some buffer space in the y-dimension (the new config options
Ly
as differentiated fromLy_analysis
) because the 1km case did not include the full 25km stretch of wetting and drying.Checklist
Testing
in this PR) any testing that was used to verify the changes