Compatibility with xgcm 0.10 + xbudget 0.7.0 - #41
Open
hdrake wants to merge 8 commits into
Open
Conversation
xgcm 0.10.0 renamed the `boundary` argument to `padding` and now raises a ValueError for `boundary`, both on the `Grid` constructor and on the axis methods (`.interp`, `.cumsum`), and exposes the per-axis setting as `axis.padding` (the private `axis._boundary` is gone). Update every site accordingly: - `Grid(..., boundary=...)` -> `padding=...` (the internal `lam`/`Z_target` grids and the docstring example) - `.interp(..., boundary=...)` / `.cumsum(..., boundary=...)` -> `padding=...` - `self.grid.axes[ax]._boundary` -> `.padding` (the `self.boundary` attribute, unused elsewhere, is renamed to `self.padding`) - the synthetic-grid test fixture's `boundary=` -> `padding=` Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xbudget 0.7.0 makes name_scheme="v1" the default in collect_budgets, which no longer fills the recipe dict's `var` fields. WaterMassBudget.__init__ relies on xbudget.aggregate(), which reads those fields and now raises on an unfilled recipe (and is deprecated). As the phased "stopgap", keep the aggregate() path: - Declare xbudget >= 0.7.0 as a direct dependency (it was only transitive via xwmt) and bump xwmt >= 0.3.0. - Silence the aggregate() FutureWarning inside __init__: the deprecation is xwmb's to act on (the planned migration to BudgetQuery), not the end user's. - Document the name_scheme="legacy" collect step in the class example, and call it in the test before constructing WaterMassBudget so aggregate() has a filled recipe to read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow xbudget 0.7.0's "convention"/"xbudget_dict" -> "recipe" rename at xwmb's boundary: WaterMassBudget now takes `recipe` (the `xbudget_dict` keyword is still accepted, warns FutureWarning, via a `_resolve_recipe` shim mirroring xbudget's), the stored raw recipe is `self.full_recipe` (with a deprecated `full_xbudget_dict` property), and docstrings/tests use "recipe". Behavior is unchanged; the old names keep working with a warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xwmb depends on xbudget's deprecated name_scheme="legacy" collect/aggregate path, which xbudget removes in v1.0. Cap the pin below 1.0.0 so a future xbudget release cannot silently break xwmb before the planned v1-native migration lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow xbudget 0.7.0's conventions/ -> recipes/ directory rename; also correct the stale `xwmt/conventions` reference (xwmb consumes xbudget recipes, not xwmt). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…shim An independent review found the xgcm 0.10 migration stopped at the package boundary, leaving the documented examples dead on arrival: - examples/load_example_model_grid.py used `xgcm.Grid(..., boundary=...)`, which raises under xgcm 0.10. Both example notebooks import this helper, so every documented example failed at grid construction. Migrated to `padding=`. - Both MOM6_water_mass_budgets notebooks called `collect_budgets(grid, recipe)` with no `name_scheme`, which under xbudget 0.7 leaves the recipe unfilled and makes the next cell raise. (The docs build hid this: nbsphinx_execute="never".) - xgcm is imported directly (`from xgcm import Grid`) but was never declared; the >= 0.10.0 requirement this branch introduces was unenforced. Declared. - The deprecation shim and the `full_xbudget_dict` property had no tests; add them (positional recipe, deprecated kwarg warns, both -> TypeError, neither -> TypeError, property read/write). `full_xbudget_dict` was a plain attribute before, so it also gets a warning setter rather than raising AttributeError. - An unfilled recipe surfaced xbudget's message pointing at BudgetQuery, which is not an option here yet. Re-raise with the step an xwmb user can actually take. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…locker Attempting to re-execute MOM6_water_mass_budgets.ipynb end to end surfaced a dependency-chain problem that the unit tests do not reach (they use the `region=None` path, never a GriddedRegion/section): sectionate <= 0.3.3 -- the latest release -- reads the private `xgcm.Axis._boundary`, which xgcm 0.10 removed in favour of `.padding`. Since this branch requires xgcm >= 0.10.0, every region/section code path raises AttributeError. The fix exists upstream but is unreleased, and the unreleased sectionate in turn drops `check_symmetric`, which the released regionate 0.5.5 still imports. No mutually compatible released combination exists today, so the notebook is left NOT re-executed and its outputs remain from the previous run. - Declare `sectionate` (imported directly, previously only transitive via regionate) with a comment recording the incompatibility; no working floor can be pinned until it is released. - Drop `metadata.kernelspec` from both notebooks so they are not tied to a registered kernel that need not exist on the machine running them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…le outputs - Pin `sectionate >= 0.3.4` instead of leaving it floorless. With no floor pip resolves to 0.3.3, which is known-broken against xgcm 0.10, turning a clean resolver error into an AttributeError deep in a call stack. Naming the first fixed version fails loudly at install time, consistent with the xbudget/xwmt floors this branch already declares. - Correct the accompanying comment. It said the section/GriddedRegion paths were unusable; in fact the default `region=None` also routes through `regionate.MaskRegions`, so *every* WaterMassBudget construction is affected. - Decouple the deprecation-shim tests from WaterMassBudget construction. They went through regionate/sectionate, whose xgcm 0.10 migration is still in flux, so they failed for environment reasons -- which masked a real mutation (a setter writing to the wrong attribute survived). They now exercise `_resolve_recipe` and the properties directly, and cover the falsy-recipe case that pins presence being tested with `is None` rather than truthiness. - Add a deprecated `boundary` property/setter. Renaming the public `self.boundary` to `self.padding` for xgcm 0.10 was an undeclared breaking change, inconsistent with the shim given to `full_xbudget_dict`. - Clear the outputs of both example notebooks. They cannot be re-executed until the regionate/sectionate chain is released, and the retained outputs showed pre-xgcm-0.10 reprs (`boundary='periodic'`) that contradict this very change. No outputs beats contradictory ones. - gitignore `data/*.nc`, which the example loader downloads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Compatibility with xgcm 0.10 and xbudget 0.7.0
Installing xbudget 0.7.0 (hdrake/xbudget#26)
pulls in xgcm >= 0.10.0 and changes xbudget's default budget engine, both of which
break xwmb. Two self-contained commits, per the phased "stopgap" approach.
1. Migrate to the xgcm 0.10
paddingAPIxgcm 0.10 renamed
boundary→paddingand now raises forboundaryon both theGridconstructor and the axis methods (.interp,.cumsum), and exposes theper-axis setting as
axis.padding(the privateaxis._boundaryis gone). Updatedevery site in
budget.pyand the test fixture; the unusedself.boundaryattribute is renamed
self.padding.2. Compatibility with xbudget 0.7.0 (
name_scheme="legacy")xbudget 0.7.0 defaults to
name_scheme="v1", which no longer fills the recipedict's
varfields;WaterMassBudget.__init__relies onxbudget.aggregate(),which reads those fields and now raises on an unfilled recipe (and is deprecated).
Keep the
aggregate()path working:xbudget >= 0.7.0as a direct dependency (previously only transitive viaxwmt) and bump
xwmt >= 0.3.0.aggregate()FutureWarning inside__init__— the deprecation isxwmb's to act on, not the end user's.
name_scheme="legacy"in the class example and the test.The hardcoded
"mass_rhs_sum_surface_exchange_flux"name is the legacy variablename, so it keeps resolving under this stopgap (it only breaks under v1 → phase 2).
Validation
pytest xwmb/tests/test_budget.py— 1 passed in the pinned env (xbudget 0.7.0,xwmt 0.3.0, xgcm 0.10.0, regionate, sectionate). The full mass-budget closure runs
on the synthetic grid.
Follow-up (not in this PR)
Migrate
__init__toxbudget.BudgetQuery(grid, recipe).aggregate(...)(works withthe default v1 output, drops the FutureWarning) and resolve the surface-exchange-flux
variable via
BudgetQuery/alias_mapinstead of the hardcoded legacy name, soname_scheme="legacy"can be dropped before xbudget v1.0.Also in this PR
xbudget_dict→recipe. Following xbudget 0.7.0's rename,WaterMassBudgetnow takes
recipeand stores the raw recipe asself.full_recipe; the oldxbudget_dict=keyword and the.full_xbudget_dictattribute still work but warn(
FutureWarning), via a_resolve_recipeshim mirroring xbudget's.xbudget < 1.0.0. This branch uses the deprecatedname_scheme="legacy"path, which xbudget removes in v1.0; the cap prevents a future xbudget release
from silently breaking this before the v1-native migration.
1. Release ordering. Neither
xbudget >= 0.7.0norxwmt >= 0.3.0exists on PyPI(latest: 0.6.3 and 0.2.0).
pip install -e .fails to resolve, so CI dies at theinstall step before pytest runs. Both must be released first.
2. regionate/sectionate are mid-migration to xgcm 0.10. Released sectionate 0.3.3
reads
xgcm.Axis._boundary, removed in xgcm 0.10. This is not limited to explicitsections: the default
region=Noneroutes throughregionate.MaskRegions, so everyWaterMassBudgetconstruction is affected. The unreleased sectionate 0.3.4 fixes itbut drops
check_symmetric, which released regionate 0.5.5 imports; with bothunreleased versions installed, a further
KeyErrorappears in the topology overhaul.sectionate >= 0.3.4is pinned so the resolver fails loudly rather than installing aknown-broken 0.3.3.
Test status:
pytest xwmb/tests/test_budget.pyis 10 passed with the releasedregionate 0.5.5 / sectionate 0.3.3. With the unreleased pair installed it is 4 failed,
3 passed — failures originate in regionate 0.6.0, not in this diff.
The example notebooks therefore cannot be re-executed; their (stale, pre-xgcm-0.10)
outputs have been cleared rather than shipped contradicting this change.
🤖 Generated with Claude Code