-
Notifications
You must be signed in to change notification settings - Fork 14
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
Enhance shared convergence steps #131
Enhance shared convergence steps #131
Conversation
TestingI have run the following convergence tests on chrys with intel, openmpi:
I reran the convergence tests on perlmutter with gnu, mpich. |
@xylar Can you take a look at this when you have a chance and let me know if you are on board with these changes before I write up the documentation? |
f8f7d67
to
9f86ada
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.
This is brilliant! Definitely a good demonstration of the shared-framework philosophy behind Polaris.
I have a few finicky suggestions but the approach looks excellent to me.
@xylar Thanks for the detailed review! I'm on board with all the changes. I'll make them and retest this week. |
polaris/ocean/tasks/manufactured_solution/manufactured_solution.cfg
Outdated
Show resolved
Hide resolved
5ca6bad
to
2de2873
Compare
Co-authored-by: Xylar Asay-Davis <[email protected]>
c37a404
to
c5a60df
Compare
c5a60df
to
d1885f9
Compare
@xylar This is ready for your (re)review. I also added a convergence suite which is convenient for testing changes to the convergence steps. |
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 ran the convergence
suite and compared with a baseline from main
on Chrysalis. The cosine bell tests are BFB with their counterparts from the cosine_bell_cached_init
suite from main
. Baseline comparison fails for the planar convergence tests because the number of time slices in the output has changed (from 31 to 2). Since I didn't see an easy way to work around this and the convergence rates look as expected, I didn't pursue this further.
@cbegeman, if you want to either clarify or remove the print statement I flagged above, I think that's the last thing needed before this can be merged. |
3a8c0e1
to
7664324
Compare
@xylar Thank you for your review! I removed the print statement and will merge once the checks are finished. |
This PR makes it possible for all convergence tests to leverage shared convergence steps and implements those shared convergence steps in all convergence tests.
The general approach is that spherical test cases will invoke
SphericalConvergenceForward
, which descends fromConvergenceForward
, andConvergenceAnalysis
and that planar test cases will invokeConvergenceForward
andConvergenceAnalysis
.The config options are moved into
convergence
andconvergence_forward
and only the default resolutions remain inspherical_convergence_forward
.Checklist
api.md
) has any new or modified class, method and/or functions listedTesting
comment in the PR documents testing used to verify the changes