Decouple OBC dynamics and tracer update#1130
Conversation
Add a 3D allocatable array `segment%dz` to `OBC_segment_type` to store per-layer vertical extents at OBC segments. This is a preparatory step to split reading dynamics and tracer data at OBC.
Split the body of update_OBC_segment_data into two new public subroutines so the dynamical fields and the tracer reservoir can be updated independently: * update_OBC_dynamics_data — normal/tangential velocity, tangential gradient, SSH, and the thickness reservoir. * update_OBC_tracer_data — tracer reservoir copy from segment%field(m)%buffer_dst to tracer registry's %t field. * update_OBC_tracer_data also has an optional logical argument include_bgc (defaulting to .true.) to allow more control. This flag will be used to preserve answers in an upcoming commit.
Replace calls to update_OBC_segment_data with two separate calls to update_OBC_dynamics_data and update_OBC_tracer_data. update_OBC_segment_data is remove from MOM_open_boundary.
Introduce read_OBC_dynamics_data and read_OBC_tracer_data, two new public routines that split the per-segment field loop in read_OBC_segment_data along its dynamics/tracer boundary (m=1..NUM_PHYS_FIELDS-2 vs m=NUM_PHYS_FIELDS-1..segment%num_fields), plus a private helper read_OBC_field_data that does the per-field read and remap. * read_OBC_dynamics_data calls thickness_to_dz and populates segment%dz and segment%dZtot, then loops over the dynamical fields. * read_OBC_tracer_data assumes segment%dz is already populated by a prior dynamics_data call at the same time step and only loops over tracer fields. * read_OBC_tracer_data also has an optional logical argument include_bgc (defaulting to .true.) to allow more control. This flags will be used to preserve answers in an upcoming commit.
Replace calls to read_OBC_segment_data with two separate calls to read_OBC_dynamics_data and read_OBC_tracer_data. read_OBC_segment_data is remove from MOM_open_boundary.
Adds a preview parameter LOCK_BGC_OBC_TO_DT_TRACER (default false) that locks BGC OBC segment data updates to every tracer advection step, ignoring DT_OBC_SEG_UPDATE_OBGC. When true, dt_obc_seg_period is forced to zero in initialize_MOM and tracer reads/updates are moved from the dynamics step (update_OBC_data) to the tracer step (step_MOM_tracer_dyn). This is a preview of a future state in which DT_OBC_SEG_UPDATE_OBGC is deprecated. All answers are bitwise identical when LOCK_BGC_OBC_TO_DT_TRACER is false (the default).
herrwang0
added
the
refactor
|
I have carefully examined this sequence of commits, and I agree with the strategy behind them and that they are correctly implemented. However, we will need to test these changes with our OBC and tracer-heavy test cases before this commit could be accepted. |
Hallberg-NOAA
added
the
Parameter change
| call get_param(param_file, mdl, "LOCK_BGC_OBC_TO_DT_TRACER", OBC%lock_bgc_obc_period, & | ||
| "If true, BGC OBC segment data is updated every tracer advection step, "//& | ||
| "ignoring DT_OBC_SEG_UPDATE_OBGC. This is a preview of a future state in "//& | ||
| "which DT_OBC_SEG_UPDATE_OBGC is deprecated.", default=.false.) |
There was a problem hiding this comment.
I find the sentence This is a preview of a future state in which DT_OBC_SEG_UPDATE_OBGC is deprecated. unclear. I think this description should clearly state when the BGC OBC segment data is updated if the flag is true (as it does) and when it is updated when it is false, I believe that would be every dynamic timestep.
There was a problem hiding this comment.
I agree and I admit this parameter itself is a bit awkward. Probably because I am jumping the gun here. Ideally, this dynamic/tracer split refactor would come more naturally, after the potentially problematic parameter DT_OBC_SEG_UPDATE_OBGC is deprecated. (hence the "preview" in the description)
| type(thermo_var_ptrs), intent(in) :: tv !< Thermodynamics structure | ||
| real, dimension(SZI_(G),SZJ_(G),SZK_(GV)), intent(in) :: h !< Thickness [H ~> m or kg m-2] | ||
| type(time_type), intent(in) :: Time !< Model time | ||
| !> Read and remap segment data for a single field index m. This is the shared per-field worker |
There was a problem hiding this comment.
I would call this a subroutine not a 'worker'
There was a problem hiding this comment.
Oh my, of course not. Thanks for catching this!
|
The parameter There are two premises, though,
I am open to suggestions. |
3 participants
This PR removes the extra read/update to OBC tracers at every dynamics time step. It also adds a parameter
LOCK_BGC_OBC_TO_DT_TRACER(default.false.) to preview the scenario that the problematicDT_OBC_SEG_UPDATE_OBGCbecome obsolete.This is a preview of a future state in which
DT_OBC_SEG_UPDATE_OBGCis deprecated. All answers are bitwise identical.How the update schedule changes
Previously, all OBC segment data, dynamics and tracers, is read and updated at the start of each dynamic step. The tracer read/update is redundant as it won't be used until the tracer advection step, which is after a multiple of dynamic steps.
DT_OBC_SEG_UPDATE_OBGCcomplicates the picture for BGC tracers, but as discussed in #1121, the only reasonable value forDT_OBC_SEG_UPDATE_OBGCisDT_TRACER_ADVECT. (DT_OBC_SEG_UPDATE_OBGC = DT also works, but it yields the same answer as DT_TRACER_ADVECT and is less efficient)The diagrams below compare the two update schedules. The answers are unchanged for either (provided that DT_OBC_SEG_UPDATE_OBGC is set correctly), but the new behavior should be more efficient.
LOCK_BGC_OBC_TO_DT_TRACER = .false.(default, existing behavior)D = dynamics; T = T/S tracer; B = BGC tracer.
DT_OBC_SEG_UPDATE_OBGC=DTDT_OBC_SEG_UPDATE_OBGC=DT_TRACER_ADVECTLOCK_BGC_OBC_TO_DT_TRACER = .true.(new behavior)T/S and BGC OBC updates are removed from
update_OBC_dataand moved tostep_MOM_tracer_dyn.DT_OBC_SEG_UPDATE_OBGCis ignored. The asterisk in the diagram below meansdzused to remap external tracers is from the previous dynamics update (at the beginning of a dynamic step, and therefore h is not updated). This is incorrect and only to keep answers unchanged. An upcoming PR will address this issue.Commits
6032018: Add a new field
dzto segments, which is used for remapping input data. This is a preparatory step to decouple dynamics and tracer updates.6569894: Split
update_OBC_segment_dataintoupdate_OBC_dynamics_dataandupdate_OBC_tracer_data6a8ce68: Apply the split for
update_OBC_segment_*c592158: Split
read_OBC_segment_dataintoread_OBC_dynamics_dataandread_OBC_tracer_datae033eb5: Apply the split for
read_OBC_segment_*bc80e75: Add parameter
LOCK_BGC_OBC_TO_DT_TRACERComments
Parameter
LOCK_BGC_OBC_TO_DT_TRACERshould be only treated as a testing tool (for timing, e.g.). If we can reach an agreement to obsoleteDT_OBC_SEG_UPDATE_OBGC, then we should simply remove allLOCK_BGC_OBC_TO_DT_TRACER=False paths, as well as some guards introduced in #1121.AI disclaimer
Various models by Claude are used to realize a small part of the code, draft commit messages, PR descriptions. Everything is checked and tested by human.