Skip to content

Conversation

@aulemahal
Copy link
Contributor

Fixes #607.

This is a very simple fix for #607 , I simply "reverted" the change in xarray's default for option keep_attrs within the function that guesses the bounds.

Another way to do this would be to use the information in Table A.1 of the conventions. I wasn't sure on how we should manage this "inherited" attributes stuff, I see two ways:

  1. No such attributes on the bounds variable, like in the file, the user should know that they are inherited from the coordinate.
  2. Explicitly inherit the attributes from the coordinate to the bounds so the user sees them on the new variable. (and change the accessor so that bounds are not included in the Axes and etc).

Method 2 seems cool but it breaks as soon as an attribute is modified on the coordinate, then the inheritance is broken. And when you write the dataset to disk, the attributes are on the variable but shouldn't be.

This is why I thought that simply dropping everything, like before, was safer.

@aulemahal aulemahal requested a review from dcherian December 5, 2025 22:07
@dcherian
Copy link
Contributor

dcherian commented Dec 5, 2025

I think the right way to deal with bounds is to "decode" them to a custom IntervalIndex or CellIndex for the 2D case

Example for time bounds: https://xarray-indexes.readthedocs.io/builtin/cfinterval.html

Analogous example for 2D cells: https://github.com/earth-mover/xpublish-tiles/blob/9afa5fea2026c5f1c48c805519f19603ae32eec1/src/xpublish_tiles/grids.py#L385

The CF thing is fine for on-disk but sucks in-memory since we can do much more complicated things in-memory

@aulemahal
Copy link
Contributor Author

I pinned pytest in this PR to "<9". I can try to fix the config for the newest version next week.

Turns out the "drop_attrs" was only needed in the 1D case as the 2D case creates a dataarray from scratch, already without any attributes.

You are right about IntervalIndexes being a better option... But I simply wanted to fix a bug here haha 😅 .

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get mypy fix and then merge.

feel free to tag a release after that

@aulemahal
Copy link
Contributor Author

@dcherian The pytest 9 issue was that the config option "python_files" changed from a space-separated string to a list. However, our value (test_*.py) was the default value, so a simple fix was to remove it.

This new type annotation for units is a bit dumb : obj: Class = Class(...), but if it helps mypy, why not. When run locally, mypy detected other simple issues that I fixed in geometry.py.

@aulemahal aulemahal merged commit 75f2dc6 into main Dec 9, 2025
11 checks passed
@aulemahal aulemahal deleted the fix-for-xr2511 branch December 9, 2025 20:04
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.

add_bounds creates broken dataset with xarray's new keep_attrs default

3 participants