Skip to content

xwmb v0.7.0: topology-aware stack, xbudget 0.8.0 recipe API, derived-variable metadata, honest budget closure - #43

Open
hdrake wants to merge 15 commits into
mainfrom
modernize-for-v0.7.0
Open

xwmb v0.7.0: topology-aware stack, xbudget 0.8.0 recipe API, derived-variable metadata, honest budget closure#43
hdrake wants to merge 15 commits into
mainfrom
modernize-for-v0.7.0

Conversation

@hdrake

@hdrake hdrake commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Supersedes #41.

Why #41 has to be replaced rather than updated

#41 was written against an unreleased intermediate xbudget commit. Its whole
strategy — xbudget.aggregate() plus collect_budgets(..., name_scheme="legacy")
targets an API that never shipped: v0.7.0 removed the dict-walking engine
outright, with no deprecation cycle, and 0.8.0 confirms it. Against the released
package:

xbudget.aggregate(recipe)                          -> AttributeError
collect_budgets(ds, recipe, name_scheme='legacy')  -> TypeError

So this PR goes straight to the v1-native API instead, and picks up the rest of the
stack while it is there.

The pinned stack

package pin why
xgcm >= 0.10.1 boundarypadding, periodic removed, cumsum(reverse=True), the north-fold boundary, multi-tile padding fixes
xbudget >= 0.8.0 the recipe API; also the UDUNITS units this PR's metadata is composed from
sectionate >= 0.4.0rc1 topology-driven section finding; f_c-aware transports
regionate >= 0.6.0rc1 boundaries are GriddedSections carrying i_c/j_c/f_c
xwmt >= 0.3.0rc1 tile-aware integration, xeos EOS, xwmt.units
xeos >= 0.2.2 now that xwmb's API exposes eos=

Naming the rc explicitly is load-bearing: PEP 440 admits a pre-release only for
specifiers that mention one, so >= 0.4.0 would silently resolve to 0.3.4. Bump each
to the final release as it lands. (xwmt itself requires xeos[teos10] >= 0.2.3, so
the resolver picks that up regardless of xwmb's >= 0.2.2 floor.)

What's in it

0. No backwards-compatibility shims. v0.7.0 breaks compatibility across the
whole stack — xgcm renamed boundary, xbudget deleted its engine outright — so a
shim in xwmb would only buy a caller the impression that they had migrated when they
had not. Every old spelling raises TypeError: xbudget_dict=recipe (now a
required positional), full_xbudget_dictfull_recipe, teos10=eos=.

default_bins also goes, but it needed replacing rather than deleting: bins
could not express what default_bins=True did (build a default target grid for this
lambda), so removing it outright would have taken a capability with it — the ECCO
example depends on it. It is now bins="default", alongside bins=<array> and
bins=None.

1. The boundarypadding migration, and arbitrary grid topology. This PR
carries modernize-topology-stack (99934ee), rebased onto current main: the
padding sweep, the split of the monolithic budget.py into single-responsibility
modules, f_c threaded through sectionate, horizontal reductions over
_horizontal_dims so they broadcast across tiles, and the ECCO LLC90 example. Three
conflicts resolved in main's favour — its git-tag versioning, and its
mass_budget(bins=...) API (which that branch predated and would have reverted; it is
now pushed down into coordinates.resolve_target_coords).

2. The xbudget 0.8.0 recipe API. WaterMassBudget builds a
BudgetQuery(grid, recipe), keeps it as self.query, and feeds
query.aggregate(decompose=…) to xwmt. Every variable name is resolved through that
query rather than hardcoded or dict-walked — hand-walking is no longer safe now that
var: null placeholders are gone and a string operand may reference the recipe's
constants: table. The argument is renamed xbudget_dictrecipe, following
xbudget; the old spelling still works and warns, via a shim mirroring
xwmt.wmt._resolve_recipe.

This fixed a live bug rather than just a name: "mass_rhs_sum_surface_exchange_flux"
is the 0.6.x spelling, matches nothing under 0.8.0, and — because the lookup was
guarded by if … in grid._ds — was silently dropping the mass source from every
budget, which in turn silently suppressed the whole closure.

3. Metadata on every derived variable. New xwmb/attrs.py. Units are composed
from the inputs with xwmt.units (a cf_units wrapper — no duplicated algebra):

mass_density  = rho_ref [kg m-3] * h [m]      -> kg m-2
layer_mass    = mass_density * areacello [m2] -> kg
mass_tendency = mass_bounds / dt              -> kg s-1

plus long_name, CF cell_methods, and the xbudget provenance of the inputs.
Following xwmt: UDUNITS-2 spellings only, and when an input's units are unknown the
units key is omitted rather than guessed, with xwmb_units_source recording
which authority answered. WaterMassBudget also warns when a grid metric is
unlabelled — xbudget multiplies areacello into nearly every term and infers units
from operands, so an unlabelled area costs the units of the entire budget, and the
published MOM6 example file ships it that way.

Two bugs surfaced here: the greater_than sign flip ran under xarray's default
keep_attrs=False and was discarding everything xwmt had just stamped; and an
identically-zero convergent transport had no units, which poisoned every sum it took
part in.

4. Not calling a residual "spurious numerical mixing" until it is one. New
xwmb/completeness.py. Anything that belongs in the budget and is missing lands in
the residual wearing the name of something it is not — silently, and with a plausible
magnitude. The audit checks the recipe's right-hand side (BudgetQuery.missing(),
incomplete_terms(), restricted to the budgets that actually feed this lambda) and
whether dM/dt, Ψ and S are present or legitimately zero (a full-domain region has no
boundary for Ψ to cross; a recipe declaring no surface mass exchange has no S to be
missing — both differ from a declared term that failed to materialize).

close_budget now always computes realized_transformation and residual,
recording absent terms in xwmb_assumed_zero, and emits spurious_numerical_mixing
only when the audit is clean. Otherwise it warns, naming each gap and the diagnostic
to go and find, and stamps xwmb_unaccounted_terms:

The tracer budget is not closed, so its residual is not attributed to spurious
numerical mixing: S (surface mass source); mass/rhs/surface_exchange_flux (missing
input(s): wfo). `residual` is still computed and carries `xwmb_unaccounted_terms`,
but it is the budget imbalance -- read it as a diagnostic of what is missing, not as
an estimate of mixing.

On a closed budget spurious_numerical_mixing is present exactly as before, so
nothing downstream changes. The report is available as wmb.completeness.

Previously this path computed nothing at all, silently, if any one of the three
terms was missing — no residual, and no explanation for its absence.

Tests

modernize-topology-stack had replaced the data-free synthetic test with MOM6-netCDF
fixtures that skip when the file is absent, which in CI meant zero budget tests
ran. Topology support is the headline of this release; it should not be exercised only
on a machine that happens to have a downloaded netCDF lying around.

xwmb/tests/synthetic.py now builds three tiny grids — plain single-tile, a bipolar
north fold (padding={"Y": {"fold": "corner"}}, with the seam vector-sign constraint
V[Ny,i] = -V[Ny,Nx-1-i] imposed so the flow is one the grid could have produced), and
two tiles joined by face_connections — following regionate's own fixtures.

The load-bearing assertion is the discrete divergence theorem: the transport xwmb
integrates along the traced boundary must equal the flux convergence summed over the
region's cells. That is exactly what a topology bug breaks, and it breaks it quietly —
a loop that misses a fold or tile seam still returns a plausible number. Both new
topologies assert it to 1e-10, as does the agreement between the along-boundary and
grid-cell-divergence methods.

Plus test_recipe_api.py, test_attrs.py, test_close.py.

Validation

  • pytest xwmb/tests -q57 passed with the MOM6 example downloaded (54 passed,
    3 skipped without it). Env: xgcm 0.10.1, xbudget 0.8.0, xwmt 0.3.0rc1, xeos 0.2.3,
    sectionate 0.4.0rc1, regionate 0.6.0rc1.
  • sphinx -b html -W on a clean tree → build succeeded. Read the Docs runs with
    fail_on_warning: true, and this branch had introduced a duplicate-object warning
    (a class re-exported at three names) plus an API page per test module.
  • The global MOM6 σ₂ budget audits as complete; spurious mixing 1.95e10 kg s⁻¹
    against a 1.94e10 kg s⁻¹ storage term. Every derived variable carries units.
  • examples/MOM6_water_mass_budgets.ipynb re-executed end to end, zero failing
    cells
    (Compatibility with xgcm 0.10 + xbudget 0.7.0 #41 had to ship its notebooks un-executed).
  • examples/ECCO_AABW_watermass_budget.ipynb (the 13-tile LLC90 AABW budget)
    re-executed end to end against the 2.7 GB Zenodo dataset, zero failing cells; that
    budget also audits as fully accounted for.

Upstream bugs found along the way

Two are in sectionate, and both are being fixed there rather than papered over here:

And one in xarray:

  • xarray 2026.7.0 raises from DataArray.chunk when the array carries a
    dask-backed coordinate of object dtype — which the published MOM6 file's cftime
    time_bounds_since_init is. Every cumulative sum failed on real model output.
    coordinates.rechunk_full rechunks only the data, which is what was wanted anyway.

🤖 Generated with Claude Code

hdrake and others added 10 commits August 2, 2026 12:26
Update xwmb to the topology-aware dependency stack and refactor the monolithic
budget.py into small, single-responsibility modules. The public API is unchanged:
WaterMassBudget(grid, xbudget_dict, region).mass_budget(lambda_name, greater_than=).

Dependencies: xgcm>=0.10.1, sectionate>=0.3.4 (#47), regionate>=0.6.0 (#22),
xwmt>=0.3.0 (#64), xbudget>=0.6.0. The whole stack now supports arbitrary
xgcm.Grid topologies, including multi-tile face_connections grids (ECCO LLC90).

Refactor:
- budget.py       thin WaterMassBudget orchestrator + mass_budget()
- regions.py      normalize any region -> .mask + .boundaries (sectionate sections)
- coordinates.py  target-coord setup, accumulate_in_lambda (cumsum reverse),
                  horizontal_grid / vertical_grid views
- transport.py    convergent transport (along-section multi-tile + divergence)
- transformations.py / mass.py / close.py  transformation, mass, closure terms

API migrations:
- boundary= -> padding= throughout; axes[ax]._boundary -> axes[ax].padding
- the four manual isel-reverse/cumsum/isel blocks -> grid.cumsum(reverse=greater_than)
- hard-coded [xc, yc] reductions -> self._horizontal_dims (broadcasts across tiles)
- region boundaries are sectionate GriddedSections carrying f_c (multi-tile aware)
- optional utr/vtr/mass_source_var so non-MOM6 conventions (ECCO) resolve transports

Add xwmb/tests (6 tests: cumsum-reverse vs numpy/manual, single-tile MOM6 global +
regional budgets, along-section == divergence agreement).

Add examples/ECCO_AABW_watermass_budget.ipynb: a full sigma2 water-mass budget for
Antarctic Bottom Water south of 30S on the native 13-tile ECCO LLC90 grid, with the
AABW class set by the abyssal minimum of the sigma2 overturning streamfunction at
30S; uses ECCO's JMD95 EOS via xeos. Adds the ECCO grid/data loader and derived
budget-term helpers. Data downloads from Zenodo (10.5281/zenodo.21479854) and is
gitignored.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RURyHPYkAZFjJ6cLRvJd3H

Rebased onto current `main`, with three conflict resolutions:

- `main`'s dynamic (git-tag) versioning is kept; the branch still carried
  `xwmb/version.py` with a hardcoded "0.6.0".
- `main`'s `mass_budget(bins=...)` API (which deprecated `default_bins`) is kept
  and pushed down into `coordinates.resolve_target_coords`, which grows a
  `bins`/`default_bins` pair and an `add_bins_gridcoords` helper. The branch
  predated that API and would otherwise have silently reverted it.
- The dependency pins are the released/pre-released topology-aware stack rather
  than the branch's provisional floors: xgcm >= 0.10.1, xbudget >= 0.8.0,
  sectionate >= 0.4.0rc1, regionate >= 0.6.0rc1, xwmt >= 0.3.0rc1, xeos >= 0.2.2.
  Naming the `rc` explicitly is what lets pip install a pre-release at all.

This commit alone does not yet run: it still calls `xbudget.aggregate()`, which
xbudget 0.7.0 removed. The next commit migrates to the recipe/BudgetQuery API.
`ci/environment.yml` installs only pytest/black/pylint/netcdf4 and leaves the
scientific stack to `pip install -e .`. That was fine when xwmb's dependency
closure was pure python, but the pinned stack is not: regionate 0.6 pulls in
geopandas/pyproj/shapely/regionmask, and xbudget 0.8's units inference needs
cf-units, which wraps UDUNITS-2 and has no usable pure-python wheel. Left to
pip, CI either builds these from source or fails outright.

List them as conda-forge packages in both the CI and docs environments so pip
finds them already satisfied.
xbudget 0.7.0 removed the dict-walking engine outright -- no deprecation cycle --
and 0.8.0 confirms it: `xbudget.aggregate()` is gone, `collect_budgets` no longer
fills the recipe's `var` fields, and derived-variable names lost their operator
infixes. A recipe is now read through `BudgetQuery`.

- `WaterMassBudget.__init__` builds `self.query = xbudget.BudgetQuery(grid,
  recipe)` and feeds `self.query.aggregate(decompose=...)` to xwmt. The query is
  kept on the instance because the budget terms resolve their variable names (and,
  in a later commit, their units) through it.
- The second parameter is renamed `xbudget_dict` -> `recipe`, following xbudget's
  own rename. The old spelling still works as a keyword-only argument and warns
  (`FutureWarning`); passing both raises `TypeError`, as does passing neither. The
  `_resolve_recipe` shim mirrors `xwmt.wmt._resolve_recipe` so the two packages
  deprecate the name on identical terms. `full_xbudget_dict` becomes a deprecated
  property over `full_recipe`.
- `transport.transport_varnames` takes the query instead of the raw recipe and
  resolves umo/vmo with `get_vars(path)["difference"][0]`. Hand-walking the recipe
  is no longer safe: `var: null` placeholders are gone, and a string operand may
  be a reference into the new top-level `constants:` table rather than a dataset
  variable name.
- `mass.MASS_SOURCE_VARNAME` (the hardcoded `"mass_rhs_sum_surface_exchange_flux"`)
  is replaced by `mass_source_varname(query)`, which asks for the term at path
  `("mass", "rhs", "surface_exchange_flux")`. This one was a live bug rather than
  a rename: under 0.8.0 the hardcoded 0.6.x name matched nothing, and because the
  lookup was guarded by `if ... in grid._ds` the mass source was *silently* dropped
  from every budget, which in turn silently suppressed the whole closure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The terms xwmb adds to the budget -- the convergent transport, the mass source,
the layer mass and its snapshots, the tendency -- all came back as bare arrays:
no units, no name, and whatever attributes xarray happened to carry over from
whichever operand was on the left of the arithmetic. A layer thickness's
`valid_range` in metres could end up on a mass in kg.

`xwmb/attrs.py` composes a description for each of them. The unit algebra is
`xwmt.units` (a cf-units wrapper) rather than a second implementation, and the
generic plumbing -- `set_default_attrs`, `strip_inherited_attrs`, `netcdf_safe`,
`prettify`, `collect_source_attrs` -- is imported from `xwmt.attrs` for the same
reason: each carries a non-obvious rationale in its docstring that a copy would
drift away from.

Units are derived, not asserted:

  mass_density  = rho_ref [kg m-3] * h [m]         -> kg m-2
  layer_mass    = mass_density * areacello [m2]    -> kg
  mass_bounds   = the same, at the time bounds     -> kg
  dt                                               -> s
  mass_tendency = mass_bounds / dt                 -> kg s-1
  convergent_mass_transport                        <- umo/vmo's own units
  mass_source                                      <- the surface flux's own units
  boundary_fluxes                                  <- the summands' common units

Following xwmt: UDUNITS-2 strings only ("kg s-1", never "kg/s"); when an input's
units are unknown the `units` key is omitted rather than guessed, and
`xwmb_units_source` records which authority answered ("source", "recipe",
"derived" or "unknown"); and summing terms whose units disagree warns and leaves
the result undescribed rather than adopting one of them.

Two related fixes found while wiring this up:

- The `greater_than` sign flip (`wmt[v] = wmt[v] * -1`) ran under xarray's default
  `keep_attrs=False`, so it discarded everything xwmt had just stamped on the
  transformation rates. Negating a quantity does not change what it is; the flip
  now runs with `keep_attrs=True`.
- An identically-zero convergent transport (the full-domain `assert_zero_transport`
  case, or a recipe with no lateral advection) is now given the mass budget's
  declared units rather than none. A zero with unknown units poisons the units of
  every sum it later takes part in.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The residual of a water mass budget is interpretable as spurious numerical mixing
only if nothing else is missing: every term that belongs in the budget and is not
there lands in the residual instead, wearing the name of something it is not. The
failure is silent and the result is plausible -- an unlabelled surface mass flux,
or a diffusion term the dataset never supplied, comes back as a confident mixing
estimate with the wrong magnitude.

`xwmb/completeness.py` audits the budget before it is closed:

- inputs the recipe names that the dataset did not supply, via
  `BudgetQuery.missing()` and `incomplete_terms()`, restricted to the budgets that
  actually feed this lambda (mass, plus the tracer budget -- or heat *and* salt for
  a density lambda). A gap in a budget that does not feed this calculation is real
  but is not a reason to distrust *this* residual, and reporting it would train the
  reader to ignore the warning.
- whether dM/dt, Psi and S are each present -- distinguishing "absent" from
  "legitimately zero". A full-domain budget has no boundary for Psi to cross; a
  recipe declaring no surface mass exchange has no S to be missing. Both differ
  from a term the recipe *does* declare that failed to materialize, which
  `BudgetQuery.var()` reports as `None` rather than raising.

`close_budget` then:

- always computes `realized_transformation` (dM/dt - S - Psi) and `residual`
  (realized - material), treating absent terms as zero and recording them in
  `xwmb_assumed_zero`. It previously returned having computed *nothing at all*,
  silently, if any one of the three was missing -- no residual, and no explanation
  for its absence.
- emits `spurious_numerical_mixing` only when the audit is clean. Otherwise it
  warns, naming each gap and the input to go and find, and stamps
  `xwmb_unaccounted_terms` on `residual`. On a closed budget the variable is
  present exactly as before, so nothing downstream changes.

The report is kept as `wmb.completeness` for programmatic inspection.

Also here, in the same "say what is wrong" spirit:

- `WaterMassBudget.__init__` warns when a grid metric carries no `units`. xbudget
  multiplies the cell area into every term it materializes and infers units from
  its operands, so an unlabelled `areacello` costs the units of the entire budget
  and everything derived from it -- and the published MOM6 example file is one of
  the datasets that ships it unlabelled.
- `mass_tendency`'s bare `print("Warning: ...")` is a real `warnings.warn`, and
  says why the first value may be NaN rather than only that it may be.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`modernize-topology-stack` replaced the data-free synthetic test with MOM6-netCDF
fixtures that `pytest.skip` when the file is absent -- which in CI means *zero*
budget tests run. The topology generalization is the headline of this release; it
should not be exercised only on a machine that happens to have a downloaded
netCDF lying around.

`xwmb/tests/synthetic.py` builds three tiny grids and a minimal recipe:

- single tile, no exotic topology (ported from `main`'s synthetic test, migrated
  to `padding=` and the 0.8.0 recipe API, and given real assertions in place of
  its bare smoke test);
- a bipolar north fold, `padding={"Y": {"fold": "corner"}}`, with the seam vector
  sign constraint `V[Ny, i] = -V[Ny, Nx-1-i]` imposed so the flow is one the grid
  could actually have produced;
- two tiles joined by `face_connections`, with the doubly-stored seam U-face made
  single-valued.

The fold and tile fixtures follow regionate's own (`test_fold_regions.py`,
`test_multitile_regions.py`) -- notably the fold's coordinate construction, where
the seam is a line between two poles so that only genuine mirror pairs coincide.
A fold-straddling region can therefore only be stitched into one boundary loop
through the topology, never through an accident of coordinates.

The load-bearing assertion is the **discrete divergence theorem**: the transport
xwmb integrates along the traced boundary must equal, to round-off, the flux
convergence summed over the region's cells. That is what a topology bug breaks,
and it breaks it quietly -- a boundary loop that misses a fold or tile seam still
returns a perfectly plausible number. Both new topologies now assert it to 1e-10,
as does the agreement between the along-boundary and grid-cell-divergence methods.

Also: `test_recipe_api.py` (the recipe/`xbudget_dict` shim, and that the transport
and mass-source names come from `BudgetQuery` rather than hardcoded strings),
`test_attrs.py` (units are composed, omitted when unknown, and never leak from a
source variable), and `test_close.py` (a declared-but-unsupplied term blocks the
spurious-mixing attribution and names the input to go and find).

The builders live in `synthetic.py` rather than `conftest.py` so the test modules
can import them directly; `conftest.py` wraps them as fixtures.

Note for anyone extending these: sectionate identifies a layer/interface coordinate
pair with `layer.replace("l", "i") == interface`, so a vertical coordinate whose
stem contains an "l" (`lam_l`/`lam_i`) is rejected. The fixtures use `sigma_l`/
`sigma_i`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things surfaced by running the suite against the downloaded MOM6 example
rather than only against synthetic grids:

- **Every cumulative sum failed on the real file.** `DataArray.chunk` rechunks the
  array's *coordinates* too, and xarray raises when it meets a dask-backed
  coordinate of object dtype -- which is what the published MOM6 file's cftime
  `time_bounds_since_init` is (`ValueError: zip() argument 2 is longer than
  argument 1`, from inside `xarray.namedarray.utils`). Rechunking coordinates was
  never the intent: a lambda accumulation needs its *data* contiguous along the
  accumulation axis. `coordinates.rechunk_full` goes through `copy(data=...)`
  instead, and the four `.chunk({dim: -1})` sites now use it.

- **Units arrived spelled two ways.** xbudget stamps `"kg.s-1"`, xwmt writes
  `"kg s-1"`. Both parse to the same unit, but a dataset whose variables disagree
  about how to spell one unit reads as though they are two. `attrs.annotate` now
  runs any units string through parse/format, so everything xwmb emits is spelled
  the same way regardless of which upstream package supplied it.

Also drops `xwmb_source_variables`, which duplicated the `xwmt_source_variables`
that `collect_source_attrs` already emits.

With the example file present the full suite is 57 passed, including the three
real-data tests; the global MOM6 sigma2 budget audits as complete and its
spurious-mixing estimate is 1.95e10 kg s-1 against a 1.94e10 kg s-1 storage term.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Both notebooks use `recipe` rather than `xbudget_dict`.
- The MOM6 notebook drops its `warnings.catch_warnings()` blocks. They were there
  to silence a `FutureWarning` that this release removes the cause of, and left in
  place they would now hide the completeness warnings a reader most needs to see.
- Two cells built regionate objects from the full budget grid, which raises under
  the new stack: sectionate traces a boundary by padding corner-index arrays over
  every axis the grid registers, and a Z axis those 2-D arrays do not span makes
  xgcm's `pad` raise `KeyError`. `xwmb.horizontal_grid` (already used internally by
  `normalize_region`) is now exported for exactly this, and the notebook uses it.
- The `MaskRegions` cell is updated for regionate 0.6: a component's boundary is a
  *list* of `GriddedSection` loops -- a region straddling a fold or a tile seam
  legitimately has more than one -- so the boundary plot iterates over them.
- `examples/load_example_model_grid.py` labels `areacello` with `units="m2"`. The
  published file ships it unlabelled, and xbudget multiplies the cell area into
  nearly every term it materializes, so without this the whole documented budget
  comes back with no units.
- README: the v0.7.0 stack, the metadata and completeness behavior, and a usage
  snippet that shows the collect-then-construct sequence rather than assuming it.
- `docs/source/install.rst` said "**xwmt** Python package for water mass
  transformation analysis"; it is xwmb, and it does budgets.

The MOM6 notebook is re-executed end to end against the pinned stack, with zero
failing cells. `docs/source/examples/` is synced (it is regenerated from
`examples/` at build time by `conf.py:_sync_examples`, but a committed copy that
contradicts the source is worse than none).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Drop the imports left behind by the refactor (`xarray` in mass.py, an unused
  test import).
- `attrs.common_units` consumed `specs` twice, so it would have silently returned
  None for a generator argument. It materializes the list first.
- `mass_budget`'s `utr`/`vtr`/`mass_source_var` docstrings still described the
  pre-BudgetQuery behavior ("defaults extract them from the budget dict", "the
  MOM6-convention name"); they now say what actually resolves them.
Running the ECCO example end to end caught a false positive in the completeness
audit that no synthetic test would have: it reported Psi as absent from a budget
that had computed it perfectly well, so the AABW notebook lost
`spurious_numerical_mixing` and died on a `KeyError`.

The audit re-derived `transport_varnames(query)` to decide whether a boundary
transport was available. But that helper looks for the MOM6 convention's
`zonal_convergence`/`meridional_convergence` term names, and the ECCOv4r4 recipe
expresses lateral advection as a `lateral_divergence` -- which is exactly why the
notebook passes `utr="umo", vtr="vmo"` explicitly. Re-deriving the names asked a
question whose answer was already known and got it wrong.

`convergent_transport_term` now stamps `xwmb_zero_reason` on the two identically-
zero returns it can produce, and the audit reads that instead of guessing:
`ZERO_BY_ASSERTION` (a full-domain region, legitimate) is told apart from
`ZERO_NO_TRANSPORTS` (a regional budget with no transport available at all, a real
gap). Both cases are covered by tests on the fold grid.

Also in the ECCO notebook: its blanket `warnings.simplefilter("ignore")` -- which is
what hid this in the first place -- now re-enables xwmb's own warnings, and the
budget audit is printed alongside the term list. `docs/source/examples.rst` gains
the notebook, which was never listed.

Both example notebooks now execute end to end with zero failing cells, and the
ECCO AABW budget audits as fully accounted for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

hdrake and others added 2 commits August 2, 2026 13:35
**No back-compat shims.** v0.7.0 breaks compatibility across the whole stack --
xgcm renamed `boundary`, xbudget deleted its engine outright -- so a shim here only
buys a caller the impression that they had migrated when they had not. All of them
are gone, and each old spelling now raises `TypeError`:

- `xbudget_dict=` and the `_resolve_recipe` shim -> `recipe` is a required
  positional argument.
- the `full_xbudget_dict` property -> `full_recipe`.
- `teos10=` -> `eos=`.

`default_bins` goes too, but it needed replacing rather than deleting: `bins` could
not express what `default_bins=True` did (build a default target grid for this
lambda), so removing it outright would have taken a capability with it -- the ECCO
example relies on it. It is now `bins="default"`, alongside `bins=<array>` and
`bins=None`, and an unrecognized string says which strings are accepted.

**Docs build.** Read the Docs runs sphinx with `fail_on_warning: true`, and this
branch introduced a warning that failed it:

    duplicate object description of xwmb.completeness.CompletenessReport,
    other instance in api/xwmb

`budget.py` was re-exporting the helpers it imports, so each was documented at three
names (`xwmb.x`, `xwmb.budget.x`, `xwmb.<home>.x`). Its `__all__` is now just the
class it defines, and the package's public surface is assembled in `__init__.py`,
importing each name from the module that defines it. The one remaining ambiguous
cross-reference is fully qualified.

Also: apidoc was publishing an API page per *test module*, because the suite gained
an `__init__.py` and became an importable package. It is excluded now. And
`docs/_build/` is gitignored.

Verified by actually building the docs, not by inspection: a fresh
`sphinx -b html -W` on this tree is **build succeeded**, with the regenerated
`api/*.rst` committed to match. `pytest xwmb/tests -q` is 57 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stored outputs were produced by the `default_bins=True` source. The code path
is identical, so the numbers do not move, but a committed notebook whose outputs
were not produced by its own source is exactly the thing that quietly goes stale.
Re-run end to end: zero failing cells, budget audit still "fully accounted for".
The previous commit's exclude pattern was written after the module path:

    main(['-f','-M','-e','-T','../../xwmb','-o','api','../../xwmb/tests'])

apidoc's signature is `[OPTIONS] -o <OUTPUT> <MODULE_PATH> [EXCLUDE_PATTERN ...]`,
and with the module path sitting before `-o` argparse cannot place the trailing
positional: it prints "unrecognized arguments" and calls `sys.exit(2)` from inside
conf.py. Sphinx reports that as

    ConfigError: The configuration file (or one of the modules it imports)
    called sys.exit()

which names neither apidoc nor the argument. Moving `-o api` ahead of the two
positionals, so they are adjacent, fixes it.

This is also a lesson about where I verified: my first check ran sphinx from
`docs/` with `source` as the source dir, which happened to take a different
argparse path and returned 0. Read the Docs runs `python -m sphinx -T -W
--keep-going -b html -d _build/doctrees -D language=en .` from inside
`docs/source`, in a conda env built from `docs/environment.yml` alone. This is
verified with that exact command in an env created from that exact file:
**build succeeded**, and `api/` contains the ten module pages with no test pages.

For the record, the three Read the Docs failures on this branch were three
different things: a duplicate-object warning (fixed two commits ago), this
`sys.exit`, and one genuine flake -- a `ConnectionResetError` while conda was
downloading packages, which no change here would have prevented.
@hdrake
hdrake force-pushed the modernize-for-v0.7.0 branch from 5b19e22 to c8ad564 Compare August 2, 2026 20:47
Sectionate is removing both arguments (hdrake/sectionate#7): the layer
coordinate is a dimension of the transports it is handed, so it reaches the
output on its own, and the matching interface coordinate is read off whichever
grid axis registers that dimension at its "center" position -- which is where
`_convergence_along_section` read the two names from before handing them
straight back.

No behavioral change. The layer coordinate arrives either way, whatever grid
sectionate is handed -- and this call hands it `hgrid`, which has no vertical
axis at all, so the `interface=` name was the only thing supplying one. It
never survived the call site regardless: the interface coordinate sits on its
own dimension, so extracting `[...]["conv_mass_transport"]` drops it. Nothing
downstream reads it; `transform_to_lambda` takes its target and target_data
from `grid._ds`.

Verified rather than argued: with sectionate's branch installed, the suite is
57 passed / 0 skipped / 0 failed, the three real-CM4p25-data tests included.
Reverting just this hunk against the same sectionate fails 4 tests with
`TypeError: ... unexpected keyword argument 'layer'`, so the along-section path
these tests cover is genuinely this call.

Must not merge before hdrake/sectionate#7 reaches sectionate `master`: until
then `convergent_transport` still defaults to `layer="z_l", interface="z_i"`,
which would mislabel a non-depth grid's output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop layer/interface from the sectionate call (unbreaks #43 against sectionate 0.4.0rc2)
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.

1 participant