+(*)Debug and correct errors with OBCs and grid rotation#900
+(*)Debug and correct errors with OBCs and grid rotation#900Hallberg-NOAA wants to merge 5 commits into
Conversation
Hallberg-NOAA
added
bug
kshedstrom
left a comment
kshedstrom
left a comment
There was a problem hiding this comment.
I can confirm that I can reproduce old answers in all my tests and get new answers in some of my tests. This looks like a great deal of work to fix the rotational tests for OBCs.
My interior_OBCs case is reproducing for all rotations, except giving a different answer with ROTATE_INDEX = False. I need to investigate more.
|
The non-rotated case is picking up values in OBC%tres_x from the initial conditions in the model while the rotated cases are picking up the values from MOM_input. Both have the correct (MOM_input) values in segment%tr_Reg%Tr(m)%tres until it gets overwritten by the 3-D restart fields from OBC%tres_[xy]. Both have the correct values in segment%tr_Reg%Tr(m)%t(i,j,k). |
|
This is probably a bug I left behind. I remember fighting hard to fix it in the rotated case. |
Hallberg-NOAA
force-pushed
the
OBC_bug_fix
branch
2 times, most recently
from
608dae7 to
d0262bc
Compare
|
This PR has been updated, and (at least in my testing) it now appears to be handling the This PR also now builds on PR #901, which ideally would be handled first. Going forward, we are thinking about refactoring the OBC code so that it is rotated from the start, thereby avoiding all of the complexity with having two separate OBC structures that need to be kept synchronized for a while. Note that the user code that specifies the time-evolving OBC values already has to be rotated, so doing the same for the initialization of the OBC segments seems like a modest added imposition on users. |
|
Although this PR is complete as it currently stands, I have converted it to "Draft" status pending the resolution of PR #901 (which this PR also contains). |
Hallberg-NOAA
force-pushed
the
OBC_bug_fix
branch
from
d0262bc to
23d1f11
Compare
|
Now all rotations of interior_OBCs match - the old non-rotated case. That means that reservoirs are being initialized with tracer values in situ, rather than tracer values from MOM_input. I was assuming we want the latter behavior, though the solutions converge in time. Note that the restart of reservoirs happens by copying values into the OBC%tres_x arrays and back again, so the timing of when OBC%tres_x is written to is what matters here. |
Revised MOM_open_boundary to better handle grid rotation for an arbitrary number of turns. This includes adding the new routine write_OBC_info() that can be used to write extensive information about the settings and the contents of arrays in the ocean_OBC_type. Because write_OBC_info() is rather verbose, it should probably only be used for debugging. Also added the new routine allocate_rotated_seg_data() and use it in place of allocate_rotated_array() in rotate_OBC_segment_data() to avoid allocating arrays with the wrong index ranges (but of the correct size). Also modified rotate_OBC_segment_data to copy over information in the tr_Reg elements of the segment when they are in use. Prior to this change, there would be a segmentation fault if the segment tracer registry was in use and the index rotation was being enabled (even if the grid was not actually being rotated). All answers in non-rotated cases are bitwise identical, and some rotated cases that previously gave segmentation faults are new running, although rotation does not yet work for all cases with open boundary conditions.
Added the new function rotate_OBC_segment_direction that returns an integer indicating the direction of an OBC segment on rotated grid, given its direction on the current grid and the number of turns by which that grid has been rotated. A negative number of tunrs undoes the rotation. All answers are bitwise identical, but there is a new publicly visible function.
Added the new publicly visible subroutine chksum_OBC_segments() for debugging open boundary conditions, and call this new routine via write_OBC_info(). Chksum_OBC_segments() does a checksum on all allocated elements of the open boundary condition segments and optionally writes out a number of layers of segment data, with the order of output chosen to appear as it would if the data were for an eastern open boundary. There are also modifications to write_OBC_info() to compensate for grid rotation so that the output should be largely invariant to rotation. This commit also adds scalar_pair arguments to 4 uvchksum calls for radiation and oblique open boundary conditions. All solutions are bitwise identical, but there is a new publicly visible interface for debugging checksums.
Added the new runtime parameter DEBUG_OBCS that triggers verbose diagnostic output about the contents of the open boundary condition type via calls to the new routines write_OBC_info() and chksum_OBC_segments(), along with the new runtime parameter NK_OBC_DEBUG to regulate the volume of diagnostic output. It also renames the previous runtime parameter DEBUG_OBC to OBC_DEBUGGING_TESTS to trigger the code that resets the normal velocities at OBC segments to zero and the thicknesses and tangential velocities behind OBC segments to silly values for testing, with DEBUG_OBC retained for now as the old name. This latter change follows the pattern elsewhere in the code in which other DEBUG parameters do not actually change any values. This commit also eliminates a fatal error message if open boundary conditions are applied with rotations of 180 or 270 degrees. All solutions in cases that worked previously are bitwise identical, but there are new runtime parameters.
Added the new runtime parameter OBC_HOR_INDEXING_BUG that can be set to true to retain previous answers or false to correct 5 horizontal indexing bugs that prevent solutions with open boundary conditions from satisfying rotational symmetry. Another 3 bugs that only apply to the ability to rotate the model but do not change answers in the unrotated model were fixed directly without there being a runtime flag to recreate the old (wrong) answers. Most of these bugs are related to the use of the brushcutter mode to set various terms in the open boundary conditions, but others the use of remapping boundary condition fields used to specify the tangential flow. Other bugs that were fixed in this commit relate to the ability to use a restart with OBCs and a rotated state, duplicated allocates for OBC tracer concentrations and reservior tracer properties, rotationally invariant checksums, and the ability to read input files onto a rotated grid. By default, all answers are bitwise identical, but there is a new runtime parameter in some MOM_parameter_doc files.
Hallberg-NOAA
force-pushed
the
OBC_bug_fix
branch
from
23d1f11 to
123f4e0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #900 +/- ##
============================================
- Coverage 37.54% 37.38% -0.17%
============================================
Files 305 305
Lines 91441 91852 +411
Branches 17416 17590 +174
============================================
+ Hits 34333 34339 +6
- Misses 50685 51092 +407
+ Partials 6423 6421 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I don't quite understand this comment, @kshedstrom . Has the revised code in this PR broken the correct previous behavior, or is this a comment on what the correct behavior should be but has never yet been implemented in this code? If it is the latter, I think that this should be noted in an issue that can be dealt with subsequent to this PR being dealt with. On the other hand, if this is a problem with the restart files not working properly, I suppose that should be dealt with now (especially if it worked before). |
|
The previous version worked for the rotated case, not the standard case (which never was quite right). Adding these lines to the end of update_OBC_segment_data gives the behavior I want (and changes dumbbell answers too): |
|
To clarify, it is not a problem with restarts (that I know of), but rather how they are implemented with the OBCs. We copy tracers into the OBC%tres_x which gets registered for restarts. Then those values are copied back into the tracer registers each timestep. The bug I've been seeing is that we copy in situ tracers into the register, then copy them into OBC%tres_x, then update the tracer values from some other initial value - based on what's in MOM_input. So you think the registers are fine, but then they get overwritten with the values from in situ tracers stored in OBC%tres_x unless you update that as well. |
|
I guess we'll have to put a bug fix flag around those lines to reproduce old answers. It isn't just dumbbell that changes, but all my coupled runs too. |
|
In order to help figure out where the answers are changing and how to add the right bug-fix flags to retain the previous answers, I have added a new PR (#904 ) that contains the debugging capabilities of this commit without any of the actual answer changing code. This should greatly reduce the volume of the code that we need to discuss further to address the issues raised with this PR. |
|
I have now added #905 to introduce the changes that allow the model to run in rotated mode with open boundary conditions, as well as a commit that corrects horizontal indexing errors (via a runtime flag that keeps the indexing errors by default). However, the are still excluding the issues related to the tracer reservoirs, which I intend to address separately. |
|
I have once again put this PR into draft status, pending the merging in of PR #907 that gives the ability to reproduce across restarts for certain cases using some types of open boundary conditions. |
2 participants
This PR consists of five commits that implement the ability to rotate (single-processor) configurations that use open boundary conditions and obtain bitwise identical solutions. It adds significant new debugging capabilities and it adds a new runtime option (
OBC_HOR_INDEXING_BUG) to correct a group of 5 horizontal indexing bugs that will change answers with OBCs, even when the solution is not rotated. Several other bugs that led to segmentation faults when the grid index rotation is enabled with OBCs were also corrected by setting the requisite pointers and copying missing data between rotated and unrotated OBC types.The specific answer changing bugs that are corrected by setting
OBC_HOR_INDEXING_BUGtofalseare 4 instances where the wrong fields are being set by reading in a file in "brushcutter" mode, and the use of the wrong total thickness when remapping tangential velocity fields or their gradients.The changes include the introduction of the new publicly visible routines
write_OBC_info()andchksum_OBC_segments()that write extensive debugging information about the OBC fields, and which were used to identify the various bugs that are being corrected with this pull request. Calls to these routines are enabled via the new runtime parametersDEBUG_OBCSandNK_OBC_DEBUG. The new functionrotate_OBC_segment_direction()is now available to convert the directions of OBC segments when there is grid rotation.A fatal error message that previously prevented cases with OBCs from running with arbitrary rotation has been removed.
The previous runtime parameter
DEBUG_OBChas been renamed toOBC_DEBUGGING_TESTS(while retainingDEBUG_OBCas the old name for backward compatability) to avoid confusion with other debugging calls that do not actually change the model state.By default, all non-rotated solutions are bitwise identical, but there are several new or renamed runtime parameters and three new publicly visible interfaces. The specific commits in this PR include: