Add average history output to driver-moab - #8637
Open
rljacob wants to merge 5 commits into
Open
Conversation
Add avg_buf_type with raw 2D arrays for time-averaging buffers. Buffer sizes are now derived from MOAB mesh cell counts and field list lengths. Remove currently unused glc, wav, and iac averaging support and eliminate dependency on component_get_c2x_cx/x2c_cx accessors.
The coupler average-history averaging period must not be smaller than the smallest component coupling period. Otherwise an averaging window can close before some components have coupled even once, leaving their fields unset. Add this check to both drivers.
The coupler average-history file did not match driver-mct. Fix four bugs; with all four the cpl.ha files are bit-for-bit with driver-mct and reproducible from run to run. Fix a dangling pointer in seq_hist_writeavg. Each component block writes a pair of fields by assigning the averaging buffer into the allocatable matrix_data and passing the pointer p_matrix to seq_io_write, but p_matrix was associated only once, before the first write of the pair. The paired field counts always differ (r2x has 8 fields and x2r 10, l2x 58 and x2l 42, and so on), so the second assignment reallocates matrix_data and frees the block p_matrix still points at. The second write of every pair therefore read freed memory. This is undefined behavior and was the source of run-to-run differences in the first averaging window, which produced a different garbage value in x2ravg_coszen_str on each run. Associate p_matrix after every assignment. This affected the atm, lnd, rof, ocn and ice blocks; the two xao blocks were already correct. Accumulate x2l from the x2l_lm snapshot taken during the land merge rather than from the live MOAB tag. Flrr_volr is mapped into x2l by prep_lnd_calc_r2x_lx in cime_run_rof_recv_post, which runs after the land merge in the same step, so reading the tag picked the new value up a step early and counted it one extra time. In the first window that made x2lavg_Flrr_volr and x2lavg_Flrr_volrmch exactly twice the driver-mct values. The instantaneous history already used this snapshot. Add a rof_coupled latch, false until the first rof coupling, and pass rofrun_alarm in from the driver. x2r tags are not written until rof couples for the first time, and with ROF_NCPL less than ATM_NCPL the first averaging window opens several driver steps before that, so seq_hist_writeavg accumulated tags that had never been set. While the latch is false x2r contributes zero, which is what driver-mct does since its x2r attribute vector is zero-filled at allocation. Zero x2r_rm in prep_rof_mrg_moab instead of reading the x2r tag back. The merge read the whole tag, overwrote only the fields it computes, and wrote the array back, so fields not computed for a given configuration, such as So_ssh when ocn_rof_two_way is off, carried the MOAB define-time default forward. This merge is the only writer of the x2r tag on mbrxid and the merge inputs are separate tags, so the read-back served no purpose. The instantaneous history never showed this because seq_io_write_moab_tags scrubs MOAB defaults on output. Co-Authored-By: Claude <noreply@anthropic.com>
rljacob
added
Coupler
Contributor
There was a problem hiding this comment.
Pull request overview
Add coupler time-average history output (cpl.ha) support and reproducibility to
the MOAB driver, aligning behavior and bit-for-bit results with the MCT driver
and addressing several MOAB-specific averaging correctness issues.
Changes:
- Add a namelist-time validation in both drivers to reject
histavgperiods
shorter than the smallest component coupling period. - Rework
driver-moabaverage-history accumulation/writing to use explicit
accumulation buffers and correct several MOAB-tag averaging pitfalls. - Fix MOAB river-merge handling to zero-fill unset
x2rfields instead of
propagating MOAB tag defaults into history output.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| driver-moab/shr/seq_timemgr_mod.F90 | Adds a histavg-vs-coupling-period sanity check to prevent invalid averaging windows. |
| driver-moab/main/seq_hist_mod.F90 | Implements MOAB average-history accumulation buffers and writing logic. |
| driver-moab/main/prep_rof_mod.F90 | Ensures x2r merge starts from zeros to avoid MOAB-default contamination. |
| driver-moab/main/cime_comp_mod.F90 | Passes ROF coupling latch into average-history writer to match MCT behavior. |
| driver-mct/shr/seq_timemgr_mod.F90 | Mirrors the histavg-vs-coupling-period sanity check in the MCT driver. |
Suppressed comments (2)
driver-moab/main/seq_hist_mod.F90:840
- Same issue in the write branch:
tag_dataallocation callsmbGetnCellson every MOAB id unconditionally, which can abort when an id is invalid for an absent component. Allocate based only on components that are present and enabled for histavg.
cnt = cnt + 1
tbnds(2) = curr_time
allocate(tag_data(max(mbGetnCells(mbaxid),mbGetnCells(mblxid), &
mbGetnCells(mbrxid),mbGetnCells(mboxid), &
mbGetnCells(mbixid),mbGetnCells(mbofxid))))
driver-moab/main/seq_hist_mod.F90:849
- Same
mbGetCellTagValsexplicit-shape /mct_string_cleanissue in the write branch. Even though onlytag_data(1:numpts)is used in the arithmetic, the fulltag_datais still passed intombGetCellTagVals, andmctOStris never cleaned.
call mct_list_get(mctOStr, ii, temp_list)
call mbGetCellTagVals(mbaxid, mct_string_toChar(mctOStr), tag_data, numpts)
a2x_ax_avg(iidx)%data(:,ii) = (a2x_ax_avg(iidx)%data(:,ii) + tag_data(1:numpts)) / (cnt * 1.0_r8)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The coupler app ids default to -1 and stay there when the component is absent, and several callers pass them without checking. The largest is the tag_data allocation in seq_hist_writeavg, which takes the max over all six coupler meshes whether or not each one exists. This does not fail today. iMOAB_GetMeshInfo indexes context.appDatas with the id, and appDatas is a std::map, so operator[] silently default-constructs an entry for -1 and returns success with zero counts. A MOSART case with stub ocean and ice, where three of the ids are -1, runs the average history code and writes its cpl.ha files. But that is correct only by accident. It relies on a MOAB container choice that is not visible from the Fortran side and would become an out-of-bounds access if appDatas ever became a vector, and it inserts a junk entry into the map on every call. The comparisons against mbaxid and mblxid further down would also match spuriously when those ids are themselves unset, so an absent component could take the vertex count branch instead of the cell count branch. Return zero early instead. No component means no cells, which is the same answer the map insertion produced, so output is unchanged: the coupled ne4pg2 case is still bit-for-bit with driver-mct and the MOSART case is unchanged across all 216 fields. mbGetCellTagVals and mbSetCellTagVals keep aborting on a bad id. Zero cells is a sensible answer for a count, but silently returning from a field read or write would hide a real failure. Co-Authored-By: Claude <noreply@anthropic.com>
mct_list_get allocates a new character buffer on every call, by way of the init() in m_List.F90, and mct_string_clean is what deallocates it. seq_hist_mod.F90 had 26 mct_list_get calls and no matching cleans, so every field of every component leaked a small buffer on every call to seq_hist_writeavg. In the ne4pg2 configuration that is roughly 350 buffers per call, so about 8000 per model day at ATM_NCPL=24 and three million per model year. Each one is small but nothing ever reclaims them, so the cost grows with run length. Add the clean after the 24 sites that consume the string immediately in a mbGetCellTagVals call. The remaining two, in the field subset block of seq_hist_writeaux, already cleaned both strings correctly including on the early exit path, and are left alone. Output is unchanged; the coupled ne4pg2 case is still bit-for-bit with driver-mct in both averaging windows. Co-Authored-By: Claude <noreply@anthropic.com>
vijaysm
requested changes
Aug 28, 2026
| ! sets every field it computes below. Fields that are not computed for this | ||
| ! configuration (e.g. So_ssh when ocn_rof_two_way is off, or the disabled | ||
| ! coszen_str) must be zero to match driver-mct, whose x2r attribute vector is | ||
| ! zero-filled at allocation. Reading the tag back instead would carry the |
| ! the smallest component coupling period. Otherwise an averaging window | ||
| ! can close before some components have coupled even once, leaving their | ||
| ! fields unset and producing wrong averages for that window. | ||
| min_cpl_dt = min(atm_cpl_dt, lnd_cpl_dt, ice_cpl_dt, ocn_cpl_dt, & |
Contributor
There was a problem hiding this comment.
Would min_cpl_dt technically correspond to the rendezvous step?
| ! ever changed). Returning here also avoids the mbaxid/mblxid comparisons | ||
| ! below spuriously matching when those ids are themselves unset. | ||
| if (moabid < 0) then | ||
| mbGetnCells = 0 |
Contributor
There was a problem hiding this comment.
Ideally, if you have an invalid MOAB appid, you shouldn't even be calling mbGetnCells. Returning 0 here could still mean that it is a valid appid, but a point cloud, for example. This is bad behavior. I'd rather throw an exception/error here so that the caller can fix their sequence.
| do iidx = 1, num_inst_lnd | ||
| do ii = 1, nflds | ||
| call mct_list_get(mctOStr, ii, temp_list) | ||
| call mbGetCellTagVals(mblxid, mct_string_toChar(mctOStr), tag_data, numpts) |
Contributor
There was a problem hiding this comment.
Does this handle both bi-grid and tri-grid cases? Does mbGetCellTagVals only get tags from "cells"?
| call mct_list_get(mctOStr, ii, temp_list) | ||
| call mbGetCellTagVals(mbaxid, mct_string_toChar(mctOStr), tag_data, numpts) | ||
| call mct_string_clean(mctOStr) | ||
| xao_ax_avg(iidx)%data(:,ii) = xao_ax_avg(iidx)%data(:,ii) + tag_data(1:numpts) |
Contributor
There was a problem hiding this comment.
I assumed that "accumulation" means technically an integration. I understand that
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add coupler time-average history output to driver-moab, and make it match driver-mct. The average history file (cpl.ha) is now bit-for-bit with the MCT driver and reproducible from run to run. This adds the avg_buf_type accumulation buffers, fixes four bugs found while comparing the two drivers against each other, hardens mbGetnCells against an unset app id, frees the mct_string buffers the averaging code was leaking, and adds a namelist check to both drivers that rejects an averaging period shorter than the smallest component coupling period.
Fixes #8502
[BFB]
Fix a dangling pointer in seq_hist_writeavg
Each component block writes a pair of fields by assigning the averaging buffer into the allocatable matrix_data and passing p_matrix to seq_io_write, but p_matrix was associated only once, before the first write of the pair. The paired field counts always differ (r2x 8 vs x2r 10, l2x 58 vs x2l 42, and so on), so the second assignment reallocates matrix_data and frees the block p_matrix still points at — the second write of every pair read freed memory.
This is undefined behavior and was the source of run-to-run variation in the first averaging window, which produced a different garbage value in x2ravg_coszen_str on each run (observed values ranged from 4.83 to 8.1e+246 to 2.8e+301). Affected the atm, lnd, rof, ocn and ice blocks; the two xao blocks were already correct.
Accumulate x2l from the land-merge snapshot
Flrr_volr is mapped into x2l by prep_lnd_calc_r2x_lx in cime_run_rof_recv_post, which runs after the land merge in the same step. Reading the live MOAB tag therefore picked the new value up a step early and counted it one extra time, making x2lavg_Flrr_volr and x2lavg_Flrr_volrmch exactly twice the driver-mct values in the first window. Now uses the x2l_lm snapshot, which the instantaneous history already used.
Zero x2r contributions until rof first couples
x2r tags are not written until rof couples for the first time. With ROF_NCPL less than ATM_NCPL the first averaging window opens several driver steps before that, so seq_hist_writeavg was accumulating tags that had never been set. Adds a rof_coupled latch (false until the first rof coupling, rofrun_alarm passed in from the driver); while false, x2r contributes zero — matching driver-mct, whose x2r attribute vector
Zero x2r_rm instead of reading the x2r tag back
prep_rof_mrg_moab read the whole x2r tag, overwres, and wrote the array back — so fields notcomputed for a given configuration (e.g. So_ssh when ocn_rof_two_way is off) carried the MOAB define-time default forward. This merge is the only writer of the x2r tag on re separate tags, so the read-back served nopurpose. The instantaneous history never showed this because seq_io_write_moab_tags scrubs MOAB defaults on output.
Return zero cells from mbGetnCells for an unset app id
Free the mct_string returned by mct_list_get
mct_list_get allocates a new character buffer on every call (via the init() in m_List.F90), and mct_string_clean is what deallocates it. Adds the clean at the 24 sites that consume the string immediately in a mbGetCellTagVals call.