-
Notifications
You must be signed in to change notification settings - Fork 60
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 nudged OBCs for tracers #194
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #194 +/- ##
============================================
- Coverage 37.22% 37.09% -0.13%
============================================
Files 261 261
Lines 72492 71836 -656
Branches 13554 13405 -149
============================================
- Hits 26985 26651 -334
+ Misses 40507 40261 -246
+ Partials 5000 4924 -76
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I approve this PR. Note that it does change answers in a few of my tests, I assume for the better. |
src/core/MOM_open_boundary.F90
Outdated
|
||
nz = GV%ke | ||
ntr = Reg%ntr | ||
|
||
if (associated(OBC)) then ; if (OBC%OBC_pe) then ; do n=1,OBC%number_of_segments | ||
segment=>OBC%segment(n) | ||
if (.not. associated(segment%tr_Reg)) cycle | ||
b_in = 0; if (segment%Tr_InvLscale_in == 0) b_in = 1 |
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.
In this case, the compilers will do the integer-to-real conversions to do the right thing, but when setting a real variable to 0 or 1, or comparing a real value to 0, the preferred syntax would be b_in = 0.0 ;
, rather than b_in = 0;
, and similarly for the other 5 instances in these two lines.
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 agree with the logic of these changes, and am convinced that they will do the right thing for tracers when tracer reservoirs are not being used. This PR should be accepted after it has passed the TC and pipeline regression testing.
There are a few minor syntax changes that I would prefer, as documented in a separate comment, but these would not change the substance of the code. In addition, there are a few places where the code is adding 3 real values without adding parentheses, but in this case these real values always take on integer values, so the order of arithmetic should not matter.
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16584 ✔️ |
squash-merging this one. |
Hi Kate
Just commit another file. The previous one had style issues.
Thanks
Wenhao
… On Aug 12, 2022, at 5:24 PM, Kate Hedstrom ***@***.***> wrote:
I approve this PR.
Note that it does change answers in a few of my tests, I assume for the better.
—
Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AW5NPO46UULYYMWYM6ETU73VY26KBANCNFSM56MVEVUA>.
You are receiving this because you authored the thread.
|
This PR is to fix nudged OBCs on tracers when both inflow and outflow length scales of tracer reservoirs are zero.
At open boundaries, tracers are updated based on tracer reservoirs. For flow entering reservoirs (exiting interior domain), reservoirs are updated with interior tracers and reservoir tracers in the previous time step. For flow existing reservoirs (entering interior domain), reservoir is updated based on nudged data applied at open boundaries and reservoir tracers in the previous time step. However, this does not work if length scales of tracer reservoirs are zero, in which case reservoirs are not updated. (@MJHarrison-GFDL )
In this fix, it is assumed that if length scale is 0, the impact of associated data is dominant, i.e.,