Refactor OBC segment tracer reservoir initialization - #1139
Conversation
herrwang0
added
the
refactor
| ! 1) It is still before remapping. | ||
| ! 2) For new runs, restart file are irrelevant. | ||
| ! 3) For restart runs, segment tracer reservoirs are assigned the value from OBC%tres_x/y in a call to | ||
| ! copy_OBC_tracer_reservoirs below in line 3735. |
There was a problem hiding this comment.
I am not confident that this call to fill_temp_salt_segments() can be moved after everything else inside of initialize_state(), nor that OBC_RESERVOIR_INIT_BUG can be eliminated altogether. This certainly was not the case when OBC_RESERVOIR_INIT_BUG was first added. There may have been subsequent refactoring that eliminated the buggy case, but it is not obvious to me what refactoring that would have been. Should I be concerned that this could be an instance of A.I. hallucination?
There was a problem hiding this comment.
Should I be concerned that this could be an instance of A.I. hallucination?
No, I am not at the stage of trusting AI to do refactor at this scale, especially for MOM6. The core of this PR is all me from earlier this year.
As for OBC_RESERVOIR_INIT_BUG, I tested with loop_current with both True and False, and the PR reproduces old answers for both. And OBC_RESERVOIR_INIT_BUG=True and OBC_RESERVOIR_INIT_BUG=False do give different answers.
Is there an old case that I should take notice? I can add it to my test suites.
There was a problem hiding this comment.
The problem occurs in cases that use ALE remapping inside of MOM_initialize_state() in the if (useALE) then block. The temperature is initialized from one of the initilize_temp_salt_...() routines, and then the reservoirs are initialized before the ALE remapping. By moving the call to fill_temp_salt_segments() outside of MOM_initialize_state(), it is only the remapped values that are available to initialize the tracer reservoirs. When I added the OBC_RESERVOIR_INIT_BUG flag in MOM_initialize_state(), I did not see any alternatives just because this particular state of temperatures and salinity is only around very briefly. When we have our consortium-wide call to change defaults, we would ordinarily be changing the default for OBC_RESERVOIR_INIT_BUG to False, but I might see whether we can jump straight to obsoleting it, which would solve all of the problems this bug creates.
I think that this strange initialization behavior that might cause problems with the changes in this PR is found in the loop_current test case.
As you do note, if the model is being restarted from a restart file, there is no problem.
There was a problem hiding this comment.
I think I found one useALE block within MOM_initialize_state (called after fill_temp_salt_segments) that could potentially be the issue. And its execution depends on parameter REGRID_ACCELERATE_INIT.
REGRID_ACCELERATE_INITis never logged.REGRID_ACCELERATE_INITis False in loop_current, which makes me wonder if this is really the culprit ...- If
REGRID_ACCELERATE_INIT=True, I got an error from loop_current restart...
MOM6/src/initialization/MOM_state_initialization.F90
Lines 488 to 510 in e1422a8
|
I have added a new PR (#1142) that should help resolve some of the concerns regarding this PR, in particular by offering a short path to obsoleting those aspects of the bugs currently recreated by |
e065251 to
45dbbc5
Compare
herrwang0
changed the title
* Rename MOM_initialize_OBCs to initialize_user_OBCs and narrow it to only set up OBC_USER_CONFIG. Its h, Time and restart_CS arguments are therefore dropped. * Faithfully move the remaining steps into the caller initialize_MOM. This includes the a few debug calls that have nothing to do with user OBC, a call to fill_thickness_segments, and most importantly, the OBC_RESERVOIR_INIT_BUG tracer reservoir setup (setup_OBC_tracer_reservoirs / set_initialized_OBC_tracer_reservoirs / fill_temp_salt_segments), which will undergo further refactor in the next few commits. * Reformat the parameter description for OBC_USER_CONFIG into an aligned list.
Remove the thickness-reservoir initialization block from initialize_OBC_segment_reservoirs and rename the routine to initialize_OBC_tracer_reservoirs to reflect that it now handles only tracer reservoirs. The removed block set segment%h_Reg%h_res from the external thickness, which is repeating fill_thickness_segments. There is currently no true external thickness value ever assigned. Besides, this block of code only works when segment%h_Reg%is_initialized was false. That condition is never reached, as fill_thickness_segments is called earlier, which initializes h_res and sets is_initialized=.true.
Simplify OBC tracer-reservoir initialization in initialize_MOM by eliminating the special-case handling that existed only for the OBC_RESERVOIR_INIT_BUG=True new-run path: - Guard the later initialize_OBC_tracer_reservoirs call with (((.not. OBC_reservoir_init_bug) .or. CS%diabatic_first) .and. is_new_run(restart_CSp)) so that on the bug route %tres set by fill_temp_salt_segments is preserved (not overwritten by %t), letting the %tres -> OBC%tres_[xy] save be deferred to the setup_OBC_tracer_reservoirs call later in initialize_MOM. - In setup_OBC_tracer_reservoirs, drop the "OBC%tres_[xy] = %t" branch taken when a segment reservoir was not yet is_initialized. The per-segment reservoir %tres is now unmodified by initialize_OBC_tracer_reservoirs in the OBC_RESERVOIR_INIT_BUG=True route, so the call to setup_OBC_tracer_reservoirs can be postponed and combined with a later call. The OBC%tres_[xy] = %t is also no longer needed. - Remove the early bug-route calls that ran right after fill_temp_salt_segments: setup_OBC_tracer_reservoirs and set_initialized_OBC_tracer_reservoirs. - Delete the now-unused public subroutine set_initialized_OBC_tracer_reservoirs and its export. With these changes %tres is no longer round-tripped through %t on the bug route, so the early setup and the %t fallback are redundant.
Restructure the OBC reservoir block that follows
tracer_flow_control_init in initialize_MOM into two mutually
exclusive branches selected by is_new_run(restart_CSp):
Previously all of these ran every time, relying on query_initialized
inside the setup_* routines and on an explicit if-guard before
copy_OBC_tracer_reservoirs to sort out the new-run vs restart cases.
The restructure makes the branching explicit, improves readability.
* new run: save segment %tres / %h_res into the global restart arrays
OBC%tres_[xy] / OBC%h_res_[xy]
* setup_OBC_tracer_reservoirs
* setup_OBC_thickness_reservoirs
* restart: restore OBC%tres_[xy] / OBC%h_res_[xy] read from the restart
file back into the segment reservoirs
* open_boundary_halo_update
* copy_OBC_radiation_coefs
* copy_OBC_tracer_reservoirs
* copy_OBC_thickness_reservoirs
OBC halo update clock init is moved from open_boundary_halo_update to
open_boundary_config.
fill_temp_salt_segments is only called before initialize_OBC_tracer_reservoirs, which is the sole place that sets segment%tr_Reg%Tr(:)%is_initialized to .true. So when fill_temp_salt_segments is called, the is_initialized flag is always .false., so the "if (.not. is_initialized)" guards is redundant. Similar for fill_obgc_segments, the only place BGC tracers' is_initialized flag is switched to true is in initialize_OBC_tracer_reservoirs, which never happens because update_OBC_seg_data=.false. during initialization. As in the previous commit, assigning BGC tracers' OBC%tres_[xy] can be deferred to a call to setup_OBC_tracer_reservoirs later in initialize_MOM.
Reorganize the open boundary condition setup in initialize_MOM simplify call sequence and improve readability. The new order is 1. Fill segment dynamics fields (unconditional): * calc_derived_thermo, * read_OBC_dynamics_data, * update_OBC_dynamics_data, * fill_thickness_segments. 2. Initialize T/S tracer reservoirs (new runs only) * First fill with interior values via fill_temp_salt_segments * Then the external read/update and initialize_OBC_tracer_reservoirs. 3. Initialize_user_OBCs, which can later override the reservoirs. The dynamics fill is moved ahead of the reservoir seeding and kept unconditional because its outputs are required regardless of the reservoir-init bug flags: the continuity calls in initialize_dyn_* consume the segment transports, read_OBC_tracer_data needs the segment%dz set by update_OBC_dynamics_data, and the update provides the one-time fill relied on when OBC_VALUE_UPDATE_BUG is true. Also remove the debug mask checksums after initialize_user_OBCs (mask2dT, mask2dC[uv], mask2dBu) and the now-unused qchksum import. There are no calls after the last mask chksum in MOM_initialize_fixed, so these debugs are not needed.
The is_initialized flag is now only read by initialize_OBC_tracer_reservoirs, and the only purpose is to skip %tres = %t copy for tracers whose reservoir value had been set explicitly via register_segment_tracer(..., OBC_scalar=...) in user cases. That guard is no longer needed after initialize_user_OBCs is now called after initialize_OBC_tracer_reservoirs, so any user-specified reservoir value is applied last and cannot be overwritten. The h_Reg (thickness reservoir) is_initialized flag is untouched.
45dbbc5 to
99d40af
Compare
2 participants
Summary
This PR aims to consolidate and clarify the initialization of OBC segment tracer reservoirs (%tres) in
initialize_MOM.The logic was previously spread across
MOM_initialize_state/MOM_initialize_OBCsandinitialize_MOM, with the new-run vs restart andOBC_RESERVOIR_INIT_BUGcases handled implicitly through scattered if-branches, which relies heavily on theis_initializedflag, and a%tres -> %t -> %tresround-trip.This PR covers mostly some preparatory work by reorder calls. The Mechanism section details the overarching logic.
The refactor results in modest reduction in line count (net -101 lines across 3 files), and better code readability for future maintenance (I think). There will be a second part that better streamlines the segment reservoir initialization and update.
Mechanism
The fundamental complication during initialization (and during the run) is the back and forth copy between per-segment tracer reservoir
%tresand global restart arrayOBC%tres_[xy]. The copy during the run was partially addressed in #1128 . This PR is the first step to addresses the initialization part.OBC tracer reservoirs exist in two parallel representations:
segment%tr_Reg%Tr(m)%tres(tracer). This is what the dynamics actuallyreads and evolves.
OBC%tres_x/y. These exist only as a buffer to be written to, and read back from the restart file.The overarching logic that motivates this cleanup:
copy_OBC_*_reservoirs: global → segment).OBC_RESERVOIR_INIT_BUGselects how the per-segment T/S reservoir is first populated: with the bug enabled (True) it is initialized from the interior field, and with the bug disabled (False) from the external (prescribed boundary) data.Commits
2d6f8f1: Rename MOM_initialize_OBCs to initialize_user_OBCs and hoist the non-user steps into initialize_MOM
834ca9b: Rename initialize_OBC_segment_reservoirs to initialize_OBC_tracer_reservoirs and drop its dead thickness block
157851e: Drop the %tres->%t round-trip and %t fallback in setup_OBC_tracer_reservoirs on the reservoir-init-bug route
3edb4ef: Split the reservoir setup/copy block into explicit new-run vs restart branches
2e7ea73: Remove redundant is_initialized guards in fill_temp_salt_segments and fill_obgc_segments
e2e5eda: Reorder OBC init so the unconditional dynamics fill precedes reservoir seeding; drop redundant mask checksums
45dbbc5: Remove the now-unused is_initialized flag from OBC tracer reservoirs
Comments to the reviewers
Because this a somewhat voluminous PR, each commit is designed to change just one thing. It is probably easier to go through the commits one by one. For each commit, I added some explanatory comments on where codes are removed, which hopefully makes the review easier.
There is no answer change. The PR is tested in loop_current test (
OBC_RESERVOIR_INIT_BUG=T/F,DIABATIC_FIRST=T/F, new/restart) and ESMG user cases.Various Claude models are used to double check code logics, and draft commit/PR messages.