Skip to content

Apply the variable defaults and the recorded time interval to AMWG tables (#423) - #476

Closed
brianpm wants to merge 1 commit into
NCAR:mainfrom
brianpm:fix-amwg-table-data-cleaning
Closed

brianpm wants to merge 1 commit into
NCAR:mainfrom
brianpm:fix-amwg-table-data-cleaning

Conversation

@brianpm

@brianpm brianpm commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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 where files are opened 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_midpoint sets the time coordinate to the midpoint
of the interval each step covers, and it is applied by adf_utils.load_dataset,
AdfData.load_dataset, AdfData.load_timeseries_dataset, create_climo_files
and AdfInfo.

Which variable holds the bounds is taken from the bounds attribute of the
time 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_bnds and
time_bounds tried. A file that records no bounds is returned untouched, since
there 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_bnds and the other time_bounds, and both declare it in the attribute.

Five 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 for time_bnds, so a model
writing time_bounds had its climatologies built from raw stamps as well.
Four are now the one shared function. 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:

before after
PRECT unit m/s mm d$^{-1}$
PRECT mean 3.37e-08 2.907
sample size 4 5

The 2.907 is the old value times the 86400000 the defaults ask for. The extra
year is the stamping fix: the average now covers the five years that were
requested, which also moves the numbers slightly (TS 288.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_bounds and stamps at the end of the interval will see their
climatologies change, because those were being built from the wrong stamps.

Testing

Unit tests: 135 pass, up from 124. Eleven new ones cover the choice of bounds
variable, including a file whose bounds attribute points at one variable
while 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 the
midpoint, 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.

One thing worth a reviewer's eye

AdfData.load_da calls squeeze() on the variable, which the tables' previous
direct read did not. For a variable with a single vertical level that would
drop the level dimension, and the table skips variables by looking for lev in
the coordinates, so such a variable would now be included where it used to be
skipped. No variable in the test configuration is like that, and it is
arguably the better behaviour, but it is a change and it is not something the
issue asked for.

…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.
@brianpm

brianpm commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Reopened as #477 from the right account; same branch and same commit. Closing this one to keep the discussion in one place.

@brianpm brianpm closed this Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AMWG tables are not getting proper data cleaning

2 participants