Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix writing of DataTree subgroups to zarr or netCDF #9677

Merged
merged 9 commits into from
Nov 4, 2024

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Oct 24, 2024

Consider a DataTree with a group, e.g.,
tree = DataTree.from_dict({'/': ... '/child': ...})

If we write tree['/child'] to disk, the result should have groups relative to '/child', so writing and reading from the same path restores the same object.

In addition, coordinates defined at the root should be written to disk instead of being omitted.

  • Tests added

Consider a DataTree with a group, e.g.,
`tree = DataTree.from_dict({'/': ... '/child': ...})`

If we write `tree['/child']` to disk, the result should have groups
relative to `'/child'`, so writing and reading from the same path
restores the same object.

In addition, coordinates defined at the root should be written to
disk instead of being omitted.
@TomNicholas TomNicholas added topic-backends topic-DataTree Related to the implementation of a DataTree class labels Oct 24, 2024
Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Good catch!

Too slow for the v2024.10.0 release but this is important so we should release again very soon.

As discussed in the last xarray meeting, this defaults to
write_inherited_coords=True, which has a little more overhead but means
you always get coordinates when opening a sub-group.
@@ -1609,6 +1610,11 @@ def to_netcdf(
group : str, optional
Path to the netCDF4 group in the given file to open as the root group
of the ``DataTree``. Currently, specifying a group is not supported.
write_inherited_coords : bool, default: True
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens at read-time? Do we normalize and remove duplicated coords by default? If so, I think we should change so that does not happen by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

what happens at read-time? Do we normalize and remove duplicated coords by default?

Yes, this is what happens.

If so, I think we should change so that does not happen by default.

Can you clarify what you mean here?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC the default is to denormalize at write-time and normalize at read-time, where "normalizing" means that coordinates are de-duplicated and inherited from parent groups where possible.

To me, that's confusing in that you really have to opt-in to round-trip to disk exactly what you have in memory. Also, anyone reading a CF-compliant store with inherited coordinates and writing it out will be surprised when opening it up with another library. FWIW it seems to me that we usually regret this kind of convenience/consistency tradeoff in the long-run.

I don't feel too strongly though. At the very least we should add some docs on how to roundtrip exactly, and how to open and write CF-compliant datasets with coordinate inheritance.

Copy link
Member Author

Choose a reason for hiding this comment

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

The advantage of writing things out this way is that you get the same DataTree from reading out a particular group, i.e.,

tree.to_zarr(path)
child_from_disk = open_datatree(path, group=child)
xarray.testing.assert_equal(tree[child], child_from_disk)

I think this will be a little less surprising to users, but overall I agree that it does not matter too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect when handling CF compliant data to roundtrip accordingly.

This might lead to issues on the user side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reverted changing this behavior -- the default is now write_inherited_coords=False.

@shoyer
Copy link
Member Author

shoyer commented Oct 30, 2024

Any further concerns here?

@dcherian
Copy link
Contributor

no conceptual concerns. Thanks.

@shoyer shoyer merged commit 577221d into pydata:main Nov 4, 2024
29 checks passed
@shoyer shoyer deleted the datatree-write-subgroup branch November 4, 2024 21:43
dcherian added a commit to dcherian/xarray that referenced this pull request Nov 7, 2024
* main:
  Enforce ruff/flake8-pie rules (PIE) (pydata#9740)
  Enforce ruff/flake8-comprehensions rules (C4) (pydata#9724)
  Enforce ruff/Perflint rules (PERF)  (pydata#9730)
  Apply ruff rule RUF007 (pydata#9739)
  chmod -x (pydata#9725)
  Aplpy ruff rules (RUF) (pydata#9731)
  Fix typos found by codespell (pydata#9721)
  support for additional scipy nd interpolants  (pydata#9599)
  Apply ruff/flake8-simplify rules (SIM) (pydata#9727)
  Apply ruff/flake8-implicit-str-concat rules (ISC) (pydata#9722)
  Apply ruff/flake8-pie rules (PIE) (pydata#9726)
  Enforce ruff/pygrep-hooks rules (PGH) (pydata#9729)
  Move to micromamba 2 (pydata#9732)
  Fix groupby tests (pydata#9716)
  Add missing xarray.core.missing import (pydata#9714)
  Fix writing of DataTree subgroups to zarr or netCDF (pydata#9677)
  Bump pypa/gh-action-pypi-publish from 1.10.3 to 1.11.0 in the actions group (pydata#9707)
  Update pre-commit hooks (pydata#9713)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-backends topic-DataTree Related to the implementation of a DataTree class
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants