-
Notifications
You must be signed in to change notification settings - Fork 374
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
Evaluate time-dependent tendencies at correct time for each RK4 stage #6695
Conversation
Testing: With this PR, the manufactured solution test case in Polaris converges at 2nd order when refining in space/time: When refining in time only, 4th order convergence is achieved: However, I have not been able to reproduce the 4th order convergence for the hurricane test case from Lilly et al. 2023 (Figure 4). The best I have been able to achieve is first order: |
Relevant discussion of this PR can be seen in E3SM-Ocean-Discussion#55 |
Tested with gnu on chicoma for both debug and optimized, compared to the master branch point, for MPAS-Ocean standalone and the compass nightly suite. I am able to get a bfb match between this branch and master. In particular, the RK4 tests are BFB:
That makes sense, because One small note: The step I also tested E3SM on perlmutter. This does not exercise the RK4 code, but it at least compiles it. E3SM passes:
|
@mark-petersen, how are you able to do standalone testing on Chicoma? There shouldn't be compatible spack packages until MPAS-Dev/compass#865 goes in. |
Hmm... I'm on the head of
etc. And I had compiled stand-alone in separate directories, not in the compass submodule. It all worked fine, so I don't know. |
I see. I'm a little surprised that works. If you were to try to recreate the load script, I don't think it would work -- at least that's what others have reported. |
I think the first two plots above show that this PR is working as intended. Those manufactured solutions provide the forcing at a requested moment in time. The hurricane test case (third plot) shows 1st rather than expected 4th order convergence. That was tricky to obtain in Lilly et al. 2023, and there must be some small difference in the way that the forcing data is interpolated in time. I'm happy for this PR to be merged despite that difference. The manufactured solution shows that this code in this PR is correct. In practice the wind data is updated at 1 or 6 hour intervals, so the model does not need interpolation that satisfies fourth-order convergence at very small time intervals. |
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.
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.
Approving on the basis of code review and improved convergence in idealized configurations. Since RK4 is not used in realistic configurations, the results of the idealized testing thus far are sufficient.
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.
Apologies! I didn't realize I was the one holding this up. I am fine with these changes, given the testing that has been done so far. Thank you for the hard work, @sbrus89!
Evaluate time-dependent tendencies at correct time for each RK4 stage This PR corrects a longstanding issue with RK4 and tendency terms which have an explicit time dependence. Big thanks to @gcapodag and @jeremy-lilly for providing the fix in their LTS development branch. Here, I have migrated these changes from their MPAS-Dev branch to E3SM and added the modifications to correct the convergence issues for the manufactured solution test case [BFB] - mpaso standalone
Passes:
merged to next |
merged to master |
This PR corrects a longstanding issue with RK4 and tendency terms which have an explicit time dependence (see #5364 and MPAS-Dev/MPAS-Model#375). Big thanks to @gcapodag and @jeremy-lilly for providing the fix in their LTS development branch linked in #5364.
Here, I have migrated these changes from their MPAS-Dev branch to E3SM and added the modifications to correct the convergence issues for the manufactured solution test case: E3SM-Project/polaris#72
[BFB] (for all standard E3SM tests)