Apply the variable defaults and the recorded time interval to AMWG tables (#423) - #477
Merged
brianpm-ucar merged 2 commits intoSep 4, 2026
Merged
Conversation
…bles The AMWG tables read their time series files directly, so the scale factor, offset and units named in the variable defaults were never applied and the tables reported the model's raw values: precipitation came out as 3.37e-08 m/s where the defaults ask for mm/day. They now load through the ADF's own data layer, which applies them (issue NCAR#423). The same issue notes that nothing corrected the time stamps. CAM stamps a monthly average with one end of the interval it covers, and which end depends on the model version: an older CAM h0 file stamps January with February 1st. Anything that then asks which year a step belongs to puts it in the wrong one, so a five year request produced a four year average, silently. Rather than correct this in the tables, it is corrected where files are opened, so that a script which does not know about the stamping convention does not have to. The bounds variable is taken from the 'bounds' attribute of the time coordinate, which is where CF says it belongs and is authoritative when present; only if it is missing or names something absent are the conventional names tried. That precedence is not academic here: of the two test cases, one records 'time_bnds' and the other 'time_bounds', and both declare it in the attribute. There were five hand-rolled versions of this in the tree, none of which read the attribute and all of which assumed the bounds dimension is called 'nbnd'. One of them, in create_climo_files, looked only for 'time_bnds', so a model writing 'time_bounds' had its climatologies built from raw stamps too. Four are now the one shared function; the two in the TEM scripts are left alone, since TEM has its own data and its own tests. Measured on a five year model against model run, for a case stamped at the end of the interval: precipitation goes from 3.37e-08 m/s to 2.907 mm/day, and the sample size from four years to five. A case already stamped mid-interval is unchanged, and the climatology files for both cases are identical to those the previous code wrote.
Review of NCAR#477 found two ways the change reached further than it should. The time stamp correction was applied wherever files are opened, which included climatology and regridded files. Those carry a time coordinate of month numbers, and turning it into dates changed files the ADF writes and reads back: a regridded file came out with a date axis, and with a different reference date per case. The data was identical, but the format of a persistent artifact is not something this change should alter. The correction is now asked for explicitly, and only where the data really is a time series: the two time series loaders, the AMWG tables and the QBO plot. Taking the units from the variable defaults also put their LaTeX into the tables, which show it exactly as it is typed: a csv row read 'mm d$^{-1}$', and so did the web page. The defaults write units for plot labels, where matplotlib renders them, so they are now resolved to plain text on the way into a table -- 'mm d^-1'. Units with no markup are untouched, so this keeps working if the defaults are rewritten in plain text, which is what pull request NCAR#427 proposes. Also from review: record adf.data in the table script's list of what it needs from the ADF object. Checked end to end: the tables read 'mm d^-1' in both the csv and the web page, the sample size is still the five years that were asked for, and the regridded files again carry the month-number axis that main writes.
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.
Fixes #423.
The AMWG tables were reporting raw model values, and the time stamps in the
time series files were never corrected, so a five year request could produce a
four year average. Both are fixed.
The variable defaults were not applied
The tables read their time series files directly with xarray, so the scale
factor, offset and units from the variable defaults never reached them. They
now load through the ADF's own data layer, which applies them.
The time stamps were not corrected
CAM stamps a monthly average with one end of the interval it covers, and which
end depends on the model version: an older CAM h0 file stamps January with
February 1st. Anything that then asks which year a step belongs to puts it in
the wrong one. In the tables that shows up as a lost year, because the annual
mean drops the two years that then look incomplete.
This is fixed in the loaders rather than in the tables, so that a script which
does not know about the stamping convention does not have to.
adf_utils.use_time_bounds_midpointsets the time coordinate to the midpointof the interval each step covers.
It is asked for explicitly, and only where the data is a time series:
AdfData.load_timeseries_dataset, both time seriesload_*_damethods, theAMWG tables, the QBO plot,
create_climo_filesandAdfInfo. The genericload_datasetin bothAdfDataandadf_utilstakes ause_time_boundsargument that is off by default, so climatology and regridded files are not
touched: those carry a time coordinate of month numbers, and turning it into
dates would change files the ADF writes and reads back.
Which variable holds the bounds is taken from the
boundsattribute of thetime coordinate first, which is where CF says it belongs and is authoritative
when it is there. Only if that attribute is missing, or names something the
file does not contain, are the conventional names
time_bndsandtime_boundstried. A file that records no bounds is returned untouched, sincethere is then nothing better to go on than the stamp itself.
That precedence is not academic. Of the two cases used for testing, one records
time_bndsand the othertime_bounds, and both declare it in the attribute.Four hand-rolled versions of this logic were in the tree, none of which read
the attribute and all of which assumed the bounds dimension is called
nbnd.One of them, in
create_climo_files, looked only fortime_bnds, so a modelwriting
time_boundshad its climatologies built from raw stamps as well.Three of them are replaced by the shared function, and the tables and the QBO
plot are new applications of it. The two in the TEM scripts are left alone:
TEM has its own data and its own tests, and widening this further would make
the change harder to review. Worth a follow-up.
What changes for users
Measured on a five year model against model run, for a case stamped at the end
of the interval:
m/smm d^-1The
2.907is the old value times the 86400000 the defaults ask for. The extrayear is the stamping fix: the average now covers the five years that were
requested, which also moves the numbers slightly (
TS288.575 to 288.544).A case already stamped mid-interval is unchanged, and the climatology files
written for both cases are identical to those the previous code wrote, so this
does not disturb runs that were already correct. Anyone whose model writes
time_boundsand stamps at the end of the interval will see theirclimatologies change, because those were being built from the wrong stamps.
Units in the tables
The units in the variable defaults are written for plot labels, where
matplotlib renders the LaTeX in them. Taking them into a table showed the
markup as typed: a csv row read
PRECT,mm d$^{-1}$, and so did the web page.adf_utils.plain_text_unitsresolves them on the way into a table, so the rownow reads
PRECT,mm d^-1. Units with no markup pass through untouched, sothis keeps working if the defaults are rewritten in plain text, which is what
#427 proposes; the two do not conflict.
Two limits worth knowing
With
compare_obs: truethe units fix is only partial, and this ispre-existing behaviour that the PR does not change.
get_value_convertersconsults the defaults only for variables in
ref_labels, which in obs mode isbuilt from the observation files actually present. So a run with
PRECTandPRECCreports the first inmm d^-1and the second inm/s, because onlyPRECThas an obs file. The unit a table reports therefore depends on whichobs files happen to be staged. Plots in obs runs already behave this way.
Worth its own issue rather than a change here.
The
create_climo_fileshalf of the time stamp fix is covered by unit testand by reading, not by a run: neither case available for testing is both
stamped at the end of its intervals and using the
time_boundsspelling,which is the combination that was going uncorrected there.
Testing
Unit tests: 138 pass, up from 124. Fourteen new ones cover the choice of bounds
variable, including a file whose
boundsattribute points at one variablewhile a differently named decoy carries the conventional name, both spellings
with no attribute at all, an attribute naming something absent, a bounds
dimension called something other than
nbnd, data already stamped at themidpoint, files recording no bounds at all, and both the noleap and standard
calendars.
End to end on Casper, model against model over five years, with the before and
after tables in the table above. The run finishes with no errors and writes its
plots, and the climatology files were compared against those from the previous
code and are identical.
On
squeeze()An earlier version of this description flagged
AdfData.load_dacallingsqueeze()as a possible behaviour change for single-level variables. Thatwas wrong:
squeeze()drops a length-one dimension but keeps the scalarcoordinate, so the table's
'lev' in data.coordscheck still sees it and sucha variable is still skipped, exactly as before. Verified directly.