Fix for MASK_OUTSIDE_OBCS with MASKING_DEPTH #752
Conversation
kshedstrom
force-pushed
the
fix_obc_maskingdepth
branch
from
a4ec069 to
ef18f5a
Compare
|
I am hopefully done changing things on this branch now. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #752 +/- ##
============================================
+ Coverage 36.63% 36.65% +0.01%
============================================
Files 278 278
Lines 84143 84182 +39
Branches 15833 15851 +18
============================================
+ Hits 30826 30855 +29
- Misses 47504 47507 +3
- Partials 5813 5820 +7 ☔ View full report in Codecov by Sentry. |
|
@kshedstrom In commit 01b0dc4 you updated the way to handle the v-direction which I think makes sense. However, you didn't change the way the u-direction is handled and left it in the form of the previous commit. I don't think this explains the MacOS fails (which @marshallward suspects is a new gnu-compiler options thing) but currently I think it breaks the rotational symmetry rule. |
There was a problem hiding this comment.
@Hallberg-NOAA earlier approved the first version, and I agree with Bob that this seems like a good fix. However, a subsequent "better" commit broke symmetry (see #752 (comment)). This seems easy to fix (apply to u- what was done to v-) but in the mean-time I'll mark this as "changes requested".
|
The symmetric version worked for EW boundaries, but not NS boundaries. The algorithm is sweeping across lines of i and is inherently non-symmetric. With non-zero turns, it would need fixing, I suppose. I'll have to investigate how the turns are actually done. |
|
I rotated the Supercritical test and Thomas's test and I stand by my current version. The failure is: One version of Thomas's test was spinning up a circulating flow while the current version has zero flow with nothing spinning it up. The previous code didn't have eta zeroed out outside the OBCs. I wouldn't be surprised if those eta values were being used somehow. Do we need to set them to some wacky value and see what happens? |
|
The Thomas test when rotated runs when compiled for debugging, but not when compiled for repro. It fails with: The line in question is: At this point in the run, OBC%rx_normal and ry_normal have not been allocated - when the rotation is in play, it is allocated otherwise. |
|
In MOM_state_initialization, there is a CS%OBC which has the r[xy]_normal allocated. There is also an OBC_in which does not have them allocated (in the rotated case). |
|
Also, Thomas' test is sensitive to zeroing out the outside eta, but none of the rest of my tests are. It must be something about the OBC choices he picked. As for the other issue, perhaps @marshallward knows why the rotated MOM_state_initialization gets one version of the OBC structure and the nonrotated ones get the other? |
|
I ran dueling debuggers and the answers do match inside the domain, spinning up a gyre and all. The Thomas test has a positive eta_outside in the Flather OBC. That drives fluid from outside to the inside. It sucks water from the point just outside the boundary, causing eta to get lower just outside the boundary without the MOM_barotropic fix, eventually making it lower than the ocean bottom. I still stand by my fix, except for the rotational weirdness. |
kshedstrom
force-pushed
the
fix_obc_maskingdepth
branch
from
d59cf6d to
52605fb
Compare
|
I'm trying a pointer fix to the OBC data structure problem. Things seem to be running... |
|
Running, maybe, but these are the diffs for the not rotated vs rotated Thomas test: Maybe I'll figure it out tomorrow. |
|
For the non-rotated case, OBC%segment(1)%field(1)%buffer_dst is allocated and set to the SSH outside value. |
|
Oh, it got allocated, set to the appropriate value, then decallocated. |
|
Is this where it populated the whole OBC_in structure, then gets rid of it as part of the rotation? @marshallward |
|
rotate_OBC_segment_data is called after the segment fields with a value use the value to fill buffer_dst. It copies over the value, but not the filled buffer_dst. |
|
Sorry @kshedstrom I was away last week and missed these pings. I will also try to look into this with you. |
|
Something is going on with the tracer reservoirs. At some point during initialization, they have the same values. Later, before step_MOM_dynamics, they don't. In fact the "vanilla" case has updated to T=7 and the rotated case has not updated and has somehow picked up that is_initialized is .false.. |
|
Summary of what I know about all this. Thomas Neumann asked first in the MOM6 forum, then in a MOM6 issue, about open boundaries within the domain. My tests for these were shown to be incomplete. I have managed to put in enough changes to make Dr Neumann happy, but the rotational tests for these things still have problems. For the rotational tests of OBCs, the model initializes parts of CS%OBC, but simultaneously keeps OBC_in. It is this latter structure which gets passed to MOM_state_initialization. Bits are then copied over to CS%OBC, but not quite all of them in all cases.
|
|
I could be done with this for now. It's working without rotations as far as I know. |
|
This PR includes numerous commits that have nothing to do with it. Please rebase this atop the latest version of dev/gfdl, which should hopefully eliminate some of the excess commits, most of which have already been merged into dev/gfdl. |
- Otherwise, the tracer values just outside the OBC get updated based on fluxes at the OBC and quickly go out of bounds of the equation of state.
- The previous version did the wrong thing at northern boundaries, at a southern corner too.
- It hasn't yet caused a blowup that I know of, but better to prevent any trouble while we're thinking about it.
Still not rototing OBCs correctly.
kshedstrom
force-pushed
the
fix_obc_maskingdepth
branch
from
ee42fdb to
ec17094
Compare
| @@ -2448,17 +2449,69 @@ subroutine btstep(U_in, V_in, eta_in, dt, bc_accel_u, bc_accel_v, forces, pbce, | |||
| haloshift=iev-ie, unscale=US%L_to_m**2*GV%H_to_m) | |||
| endif | |||
|
|
|||
| do j=jsv,jev | |||
There was a problem hiding this comment.
I see where this is going, but I have 4 specific problems with the changes to MOM_barotropic.F90:
- This code setting
factordoes not change from one barotropic timestep to the next, and it might not even change between calls to btstep if the OBC segments don't change. It should be moved before the barotropic time-stepping loop that starts at line 1815, and perhaps it should be made into an element ofbarotropic_CSand set inbarotropic_init(). - Why isn't there a similar change from using
CS%IareaTtofactorwhereeta_predis calculated on lines 1928, 1942 or 1948? factoris not a very specific name that it does not reflect what this array does. Something likeIareaT_OBCmaskwould reflect what this array contains, even if it is somewhat less pithy than I would like.- The comment describing factor does not indicate that its units are
[L-2 ~> m-2].
There was a problem hiding this comment.
You have some good points about the factor code. The BT_OBC structure is set up for each call to btstep, so if we don't fix that, it will be hard to move anything to barotropic_init().
| @@ -2897,6 +2897,26 @@ subroutine initialize_MOM(Time, Time_init, param_file, dirs, CS, & | |||
| ! reservoirs are used. | |||
| call open_boundary_register_restarts(HI, GV, US, CS%OBC, CS%tracer_Reg, & | |||
| param_file, restart_CSp, use_temperature) | |||
| if (turns /= 0) then | |||
There was a problem hiding this comment.
We are getting segmentation faults in the TC testing from code related to the rotated tracer reservoirs, which means that we can not accept this PR because all future PRs will also fail testing.
I suspect that the problem might be that when there are is rotation by 1 or 3 turns, we should have OBC_in%tres_x corresponding to CS%OBC%tres_y and vice versa. However, when there is rotation, these are (grid-space) arrays with different orientations and sizes so one can not simply point to the other. There has to be an explicit copy instead. These are scalars, so the sign will not change.
The specific problem that is leading to a segmentation fault might be something as simple as the fact that the pointers are just not nullified when they are declared on line 372 of MOM_open_boundary.F90, or it could be that by setting a pointer to point to another pointer, the associated test appears to be true, even if the pointer that is pointed to is not itself pointing to allocated data.
The same issues would be true of OBC_in%rx_oblique_u and the other fields, but these are only used for restarts, and I am pretty sure that these fields are never used from within OBC_in, so they might not even matter.
Also, although the OBC type is not opaque, I think that we should treat it as though it were to the extent possible, so if it does still prove necessary to retain this code, I think that this whole block of code should be moved into a subroutine in MOM_open_boundary.F90 that copies these fields from one ocean_OBC_type to another.
|
|
||
| ! Only doing this here for 1 turns, get flow directed in the opposite direction. | ||
| ! Currents that are now E_W were N_S and the turns should change their sign. | ||
| if (turns == 1 .and. segment%is_E_or_W) then |
There was a problem hiding this comment.
I believe that perhaps the logic here should be:
qturns = modulo(turns,4)
if ( (qturns == 2) .or. (qturns == 1 .and. segment%is_E_or_W) .or. (qturns == 3 .and. segment%is_N_or_S) ) then
There was a problem hiding this comment.
Thank you very much for doing the rebase, @kshedstrom . That makes it so much easier to see what is going on with this PR!
I have added some questions and thoughts about what might be going on here and some things to try that might fix our testing failures, if you are feeling up to it.
Another option here might be to break up this PR into the first few commits that we can accept without controversy (namely those related to setting MASKING_DEPTH for the OBCs and masking out the tracer-point changes in the barotropic solver and tracer advection), and then circling back to sort out what is happening with the rotation later. (The commit changing the allocatable arrays into pointers seems to me like the primary suspect for the TC testing failures, based on the error messages we are getting with our TC testing failures. To see the errors, click on the red-x's that we are getting at #752.)
|
Good idea to try without the rotational fixes for now. |
The MASK_OUTSIDE_OBCS flag doesn't know about the MASKING_DEPTH and this should take care of the problem.