ENH: Add QC- and bounds-aware data transformations - #1060
Open
maxwelllevin wants to merge 30 commits into
Open
maxwelllevin wants to merge 30 commits into
maxwelllevin wants to merge 30 commits into
Conversation
Adds a new act/transform subpackage providing bin_average, interpolate, and subsample transforms plus a transform_dataset convenience function, ported from https://github.com/maxwelllevin/act-transform. Every transform is exposed both as a plain function (act.transform.bin_average(...)) and as a Dataset accessor (ds.transform.bin_average(...)) that forwards to the same function, with no duplicated logic. The core kernels are JIT-compiled with numba but fall back transparently to a pure-Python implementation (with a warning) if numba is unavailable or fails to compile, since numba is a required-but-not-fully-stable dependency. Output QC variables are annotated with the same flag_masks/flag_meanings/flag_assessments/standard_name convention used by act.qc.qcfilter, so transform output composes with ds.qcfilter/ds.clean. This is a first-draft port intended to seed maintainer discussion, not a finalized design. Co-Authored-By: Claude <noreply@anthropic.com>
interpolate() and subsample() raised UFuncTypeError when given a real datetime64 coordinate, even though resampling onto a different time base is the subpackage's headline use case and what every docstring example shows. transform_1d passed the raw coordinate arrays straight to the kernels, which do plain arithmetic on them. bin_average avoided this only incidentally, because it routes its coordinates through _get_bounds(). _get_bounds() also rejected the dtype-object cftime.DatetimeGregorian bounds that act.io.arm.read_arm_netcdf produces: it decodes with use_cftime=True and converts only time/time_offset back to datetime64, leaving time_bounds as cftime objects. That broke explicit input_bounds and transform_dataset's CF bounds auto-detection for every ARM file carrying time_bounds. Reading the same file with plain xr.open_dataset gives datetime64 bounds and works, so it failed specifically through ACT's own reader. Add a shared _to_numeric() helper and apply it to the coordinates for all three transforms and to both branches of _get_bounds(), so every path normalizes consistently with what bin_average already did. Values are normalized to nanoseconds rather than cast blindly, so bounds and coordinates carrying different datetime units stay mutually comparable -- previously datetime64[s] bounds against a datetime64[ns] coordinate silently produced all-missing output. Numeric coordinate behavior is unchanged. Also fix two adjacent issues on the same axis: - A raw numpy datetime64 target (not a DataArray) silently degraded the output coordinate to float64; it now stays a datetime. - transform_dataset tried to transform the CF bounds variable itself as if it were measured data. Bounds describe coordinate cells, are consumed as bounds, and are not necessarily numeric, so they are now skipped. t_range may now be given as a timedelta when the coordinate is a datetime. Tests: parametrized regressions push datetime64 coordinates through all three transforms, assert the results match the equivalent numeric-nanosecond axis exactly, and cover explicit datetime64 and cftime bounds plus transform_dataset CF bounds auto-detection. The cftime paths are additionally exercised through act.io.arm.read_arm_netcdf so the reader-shaped dtype-object bounds are tested as users actually get them. 14 of the 17 new tests fail against the previous code; the 3 that pass are the bin_average cases that worked incidentally.
Adds docs/source/userguide/transform.rst and registers it in the userguide toctree. The subpackage previously had only NumPy-style docstrings and a single 'transform' line in the API index, with no narrative documentation. Covers: when to reach for act.transform instead of xarray's .resample()/.interp() and when not to; a comparison table for choosing among bin_average, interpolate, and subsample; the QC story end to end (passing input QC with a qc_mask, deriving the mask from CF flag attributes, decoding output bits, and composing with ds.qcfilter); bounds awareness and the std_*/goodfrac_* coverage thresholds; whole-Dataset use via transform_dataset including qc_ auto-pairing and per-variable overrides; the function and accessor forms side by side; datetime coordinate handling; and the numba dependency with its pure-Python fallback. Every code snippet in the page was executed against gucmetM1.b1.20230301 via arm_test_data, and the QC bit table was checked against act/transform/constants.py. Co-Authored-By: Claude <noreply@anthropic.com>
Creates examples/transform/ with a readme.txt following the pattern of
examples/corrections and examples/qc, plus four Sphinx-Gallery scripts. All are
named plot_*.py and produce a matplotlib figure, as docs/source/conf.py's
'filename_pattern' requires for gallery rendering.
plot_bin_average_resample.py 1-minute to 30-minute bin average, plotting
input against output and quantifying the
effect of the file's declared time_bounds.
plot_transform_qc_propagation.py Input QC excluded from the computation and
output QC bits generated, with the excluded
input per bin plotted against the resulting
QC_SOME_BAD_INPUTS flags.
plot_compare_transforms.py All three transforms on one target, over a
continuous temperature and a categorical
present-weather code.
plot_transform_dataset.py Whole-Dataset transform with per-variable
transform and kwargs overrides.
All four use arm_test_data's DATASETS.fetch('gucmetM1.b1.20230301.000000.cdf'),
so they need no ARM credentials. Every script was executed and confirmed to
produce its figure. ruff, black, and isort pass.
Co-Authored-By: Claude <noreply@anthropic.com>
The public functions each had a single-line Examples block showing only the
simplest call. autosummary_generate is on, so these feed the generated API pages
directly; expand them where a fuller snippet genuinely helps.
bin_average input QC with qc_mask, explicit CF bounds, and the
std_*/goodfrac_* coverage thresholds with the bits each sets
interpolate t_range as a timedelta, and reading QC_INTERPOLATE
subsample why it suits categorical variables, and QC_NOT_USING_CLOSEST
transform_dataset per_var_transform / per_var_kwargs overrides, and target_ds
Transform accessor-versus-function equivalence and the qc_var_name
argument-name difference
make_coord the numeric form, and that it excludes stop where the
datetime form includes it
Also normalizes the indentation of these blocks. They previously indented
'.. code-block:: python' one level under the Examples heading, which renders as a
blockquote; ACT's prevailing style places it flush (see
act/utils/data_utils.py:538).
Every snippet added here was executed against gucmetM1.b1.20230301 via
arm_test_data. No behavior changed; tests/transform/ still passes 44 tests.
Co-Authored-By: Claude <noreply@anthropic.com>
docs/source/API/index.rst already listed 'transform', but the generated page was
missing bin_average, interpolate, and subsample -- the subpackage's three
headline functions. Only make_coord, transform_dataset, and Transform appeared.
sphinx.ext.autosummary builds a module's member list from dir() (via
members_of() in sphinx/ext/autosummary/generate.py), and lazy.attach() derives
its __dir__ solely from the names it was asked to load lazily. The three
transforms are imported eagerly at the top of act/transform/__init__.py -- to
stop a later 'import act.transform.bin_average' from clobbering the function
with its module -- so they were absent from dir() and silently dropped from the
docs. __all__ already accounted for them, so 'from act.transform import *' and
every documented call worked; only discovery was broken, which is why nothing
caught it.
Define __dir__ to report __all__. Verified with a minimal Sphinx build using this
repo's own autosummary module template: before, generated/ held
act.transform.{make_coord,transform_dataset,Transform}.rst; after, it also holds
act.transform.{bin_average,interpolate,subsample}.rst.
Tests: TestPublicSurface asserts dir() matches __all__, that each public name is
discoverable rather than merely reachable, and that every QC bit constant is
re-exported. 4 of the 8 new tests fail against the previous code.
No behavior change -- lazy attribute access, eager imports, and star-imports all
still resolve identically.
Co-Authored-By: Claude <noreply@anthropic.com>
Extends the existing lazy_loader entry with the documentation-visible consequence of importing a name eagerly: it leaves dir(), and autosummary builds API pages from dir(), so the name vanishes from the generated docs without anything failing. Points at the __dir__ override and the test that guards it. Co-Authored-By: Claude <noreply@anthropic.com>
maxwelllevin
requested review from
AdamTheisen and
zssherman
as code owners
September 11, 2026 16:04
AdamTheisen
reviewed
Sep 18, 2026
AdamTheisen
left a comment
AdamTheisen
left a comment
Collaborator
There was a problem hiding this comment.
Overall, this looks to be a great addition to ACT that will help with ARM's datasets. I did some testing and came across a few things but overall the results were looking good and emphasized why using actual bounds matter. Let us know what you think on these comments.
Preserve the target grid's bounds variable when available and drop the bounds attribute otherwise, so the output never references a missing variable. Co-Authored-By: Claude <noreply@anthropic.com>
bin_average and interpolate kernels return a negative status on failure (e.g. -5 for input and target ordered in opposite directions), which was silently discarded and produced all-missing output with QC=0. Co-Authored-By: Claude <noreply@anthropic.com>
QC constants now use ARM's bit numbering, so drop the stale bit remapping and ignore mask. Only QC_SOME_BAD_INPUTS is ignored, since ARM sets it even when every input is bad. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Thanks @AdamTheisen for the feedback! I think I've fixed (with the help of claude) those first two issues. I need to think about the circular transform case a bit more; I think it would be a good thing to support, and to clearly document as a special case. We would need to add some additional logic for both the bin averaging and linear interpolation kernels. In making these fixes I also noticed some other issues with QC outputs compared to ARM. I'm going to work a bit more on this PR over the weekend and I'll provide updates here |
libtrans subsample starts with status 0; the port started at -1. With the status corrected, check the subsample kernel status like the others. Co-Authored-By: Claude <noreply@anthropic.com>
- Raise when a QC variable lacks flag_masks and no qc_mask is given, instead of silently excluding nothing. - Raise on wrong-length weights; the numba kernel would read past the end. - Raise on zero-width bin_average output bins, as libtrans does. - Require a timedelta t_range for datetime coordinates (and a number otherwise), since a bare number was read as nanoseconds. - Default t_range to no limit, matching libtrans, instead of the median input spacing. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…aset transform example
Contributor
Author
|
A few additional updates:
|
This branch has not been deployed
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
2 participants
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.
What changed
act.transformsubmodule providing QC- and bounds-aware:.transformxarray dataset accessor.datetime64andcftimecoordinatesWhy
Xarray’s built-in transformation methods do not natively account for quality control variables. In addition,
xarray.resample().mean()considers only coordinate values and not coordinate bounds. It therefore assumes that each measurement belongs entirely to a single output bin, which can produce incorrect results when measurements span bin boundaries.These transformations address both issues while following the behavior of ARM’s existing
libtransimplementation.Dependency considerations
The implementation uses
numbato JIT-compile the loop-based transformation logic. The code falls back to regular python execution withoutnumba, which may be significantly slower. Makingnumbaan optional dependency could be considered.Issues closed
Validation
.transformdataset accessor behaviorcftimecoordinate/bounds handlingAI usage & manual verification
Screenshots
Details