-
Notifications
You must be signed in to change notification settings - Fork 364
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 AB2 (mpas_ocn_time_integration_split_ab2.F) code #6323
Conversation
* Some codes & routines were updated in split.F, but not in split_ab2.F * 'ocn_diagnostic_solve_wctEdge' routine was updated, but split_ab2.F was using a call sentence of the old version of the routine. - 'deltaSSH' computation was missing in split_ab2.F. - 'baroclinicThickness' is computed instead of 'bottomDepthEdge'. * 'Direct application of tidal boundary condition', which was only in split.F, is added to split_ab2.F.
|
Passed all COMPASS nightly suite on Perlmutter with GNU compiler.
|
!$omp end parallel | ||
|
||
call ocn_diagnostic_solve_wctEdge(btrvel_temp, deltaSSH, & | ||
baroclinicThickness, wctEdge) |
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.
@hyungyukang commented in a meeting that this is the critical change in this PR.
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.
It looks from #6047 that this would be BFB when the thickness is centered, so I don't think this will affect the instability we encountered in the v3 HR G-case.
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 , thanks for your comment. When I tested this branch very first time, I was able to see some differences in simple test cases. So I thought this change is critical. But today, I cleaned and compiled again after the meeting, and it is very similar now. Maybe I did something wrong during early tests. However, for code uniformity between split_explicit_ab2.F
and split_explicit.F
, this PR is worth merging.
This PR is not BFB with very small differences (~1e-9).
Just in case, I will test this PR a bit more.
@hyungyukang, it would be better to separate this PR into two PRs: one that is BFB on all tests with the majority of changes, and one with the remaining non-BFB lines. That way it is clear what code changes are non-BFB. It looks like many of these changes are clean-up and conforming to the split-explicit code, but are not changing the computation. It is very helpful to have non-BFB PRs with small isolated changes, even just a few lines, when we look back later and ask how it changed the results. |
@mark-petersen , Thanks for suggesting. Agreed. I will close this PR first, then open one PR with BFB on all tests. Maybe it'd be better to open another PR after merging the BFB PR. Thanks! |
Closing this PR and splitting it into two PRs to clarify the BFB/non-BFB changes as suggested by a reviewer. |
This PR updates AB2 time stepping code.
Some codes and a routine were updated in
split.F
, but not insplit_ab2.F
ocn_diagnostic_solve_wctEdge
routine was updated, butsplit_ab2.F
was using a call sentence of the old version of the routine.deltaSSH
computation was missing insplit_ab2.F
.baroclinicThickness
is computed instead of 'bottomDepthEdge'.split.F
, is added tosplit_ab2.F
.