-
Notifications
You must be signed in to change notification settings - Fork 66
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
Fix interior obcs #814
Fix interior obcs #814
Conversation
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 are a handful of issued related to the use of wide halos in the barotropic solver that will need to be addressed (see the detailed comments), but otherwise this PR is looking good to me.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #814 +/- ##
=============================================
- Coverage 38.15% 21.91% -16.25%
=============================================
Files 296 136 -160
Lines 87322 32868 -54454
Branches 16303 5847 -10456
=============================================
- Hits 33316 7202 -26114
+ Misses 48012 25109 -22903
+ Partials 5994 557 -5437 ☔ View full report in Codecov by Sentry. |
Thanks for pointing out that the array needs wide halos. Hopefully this takes care of the problem - I don't have wide halo tests since they aren't compatible with OBCs. |
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 am convinced that this revised PR is now correct as written.
The one point about this commit that is still troubling me is that some diagnostics are changing slightly, so this PR is tripping over the automated TC testing. Moreover there does not appear to be way to recover the previous answers. If we were to add a new runtime parameter (like EXTERIOR_OBC_BUG
) and a logical bug-fix test (like changing line 4900 of MOM_barotropic.F90 to if (associated(OBC) .and. (CS%exterior_OBC_bug == .false.)) then
and something similar at lines 652 and 1056 of MOM_tracer_advect.F90, and perhaps add some logical tests at about lines 5079-5080 of MOM_open_boundary.F90, could we recover the previous answers with the right run-time parameter settings?
If such adding the new runtime parameter did work recover the previous (incorrect?) answers, it would greatly ease my unease about bending the rules for our testing, and I would by very happy to accept this PR without reservations.
0171c8d
to
a618661
Compare
Thank you for the suggestions! I will add the logic to the MASKING_DEPTH code if it fails the tc testing. |
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.
With the recent changes to this PR, all of my previous concerns have been addressed.
070e36c
to
2171723
Compare
This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26527 with the expected warnings about a new runtime parameter. |
Just the non-rotational parts of PR #752 for now.