Fix for MASK_OUTSIDE_OBCS with MASKING_DEPTH #752
Conversation
- 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.
kshedstrom
force-pushed
the
fix_obc_maskingdepth
branch
from
a4ec069 to
ef18f5a
Compare
- 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.
|
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.
|
Fix a bug with subroutine write_energy when using a DT<2. Otherwise, the energy outputs are written at wrong time steps. The reason was that time type divide is essentially a floor. So DT/2 = 0 if DT<2.
The subroutine compute_global_grid_integrals appeared in both the MOM_state_initialization and MOM_shared_initialization modules, but was only being called from the latter. This commit removes the extra copy in MOM_state_initialization. It also removes some unnecessary parentheses in the copy that is being retained, in part to facilitate the review of this commit. All answers are bitwise identical, and no publicly visible interfaces are altered.
In addition to REMAPPING_USE_OM4_SUBCELLS, for ALE remapping, there are several parameters of the form XXX_REMAPPING_USE_OM4_SUBCELLS, where XXX identifies the target, and they all currently default to True. To simplify setting them all to False, which is recommended, the defaults for the XXX versions is changed to the value of REMAPPING_USE_OM4_SUBCELLS. Answers are only changed if REMAPPING_USE_OM4_SUBCELLS is set to False and the default (now False) is used for one or more of the other parameters. In such cases the original behaviour can be recovered by explicitly setting the other parameters to True.
Removed two instances of `fail_if_missing=.true., default=0.` which are contradictory: a default value is meaningless if the parameter must be specified. I encountered this when adding the `defaults=` option to `get_param_real_array()`.
The `default=` optional argument to get_param() only provides a uniform value to initialize an array of reals. This commit adds the optional `defaults=` argument that must have the same length as the `values` argument. I've also added a few instances of this optional argument: - by adding the `initialize_thickness_param()` procedure, selected by `THICKNESS_CONFIG = "param"`. The procedure was based on the "uniform" method, and uses the parameter `THICKNESS_INIT_VALUES` which defaults to uniform values derived from `MAXIMUM_DEPTH` - the setting of MLD_EN_VALS in MOM_diabatic_driver.F90 which was previously using a work around to set defaults to 25, 2500, 250000 J/m2. - two vectors of 4 values in user/user_change_diffusivity.F90 There will be some doc file changes, but no answer changes.
Two latent heat constants are imported directly from FMS, which is built independently of MOM6. Previously, it was a safe assumption that both would be built with double precision, but this is no longer the case since FMS now supports both single and double precision. This could cause conflicts with mixed-precision builds. This patch converts the values from FMS-precision to MOM-precision. Single->double should not affect reproducibility since every single-precision number can be exactly represented in double precision. Double->single could affect reproducibility, but this is not an issue since MOM6 does not run in single precision.
* Inline harmonic analysis
Important bug fix:
1) The Cholesky decomposition was operating on entries below
the main diagonal of FtF, whereas in the accumulator of FtF, only
entries along and above the main diagonal were calculated. In this
revision, I modified HA_accum_FtF so that entries below the main
diagonal are accumulated instead.
2) In the accumulator of FtSSH, the first entry for the mean
(zero frequency) is moved out of the loop over different tidal
constituents, so that it is not accumulated multiple times within
a single time step.
* Inline harmonic analysis
Another bug fix: initial state added back to the mean state.
* Inline harmonic analysis
Minor update to HA_solver
* Tidal angular frequency has units [rad s-1]
Tidal frequencies are always angular frequencies to simplify applying
sine and cosine. These have MKS units [rad s-1] but they are all
currently listed as [s-1].
Updated dOxygen comments for variables, e.g. [T-1 ~> s-1] becomes
[rad T-1 ~> rad s-1]. Updated get_param units. e.g. units="s-1"
becomes units="rad s-1".
No answers are changed, but the logged parameter units are different.
There are frequencies in MOM_internal_tides.F90 but these have not
been updated because they may be specified incorrectly. They are
used as if they are [T-1] but they are calculated as 2PI/period [rad T-1].
real, allocatable, dimension(:) :: frequency !< The frequency of each band [T-1 ~> s-1].
real :: period ! A tidal period read from namelist [T ~> s]
! The periods of the tidal constituents for internal tides raytracing
call read_param(param_file, "TIDAL_PERIODS", periods)
do fr=1,num_freq
period = US%s_to_T*extract_real(periods, " ,", fr, 0.)
CS%frequency(fr) = 8.0*atan(1.0)/period
enddo
All MOM6-examples cases have INTERNAL_TIDES=False and so can't
resolve this issue.
* fixed too-long line
|
I could be done with this for now. It's working without rotations as far as I know. |
The MASK_OUTSIDE_OBCS flag doesn't know about the MASKING_DEPTH and this should take care of the problem.