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

Enable datatree * dataset commutativity #7497

Closed
wants to merge 26 commits into from

Conversation

TomNicholas
Copy link
Member

Change binary operations involving DataTree objects and Dataset objects to be handled by the DataTree class. Necessary to enable ds * dt to return the same type as dt * ds.

Builds on top of #7418.

TomNicholas and others added 26 commits January 4, 2023 15:20
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
@TomNicholas TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label Feb 1, 2023
@github-actions github-actions bot added CI Continuous Integration tools dependencies Pull requests that update a dependency file labels Feb 1, 2023
Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

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

Looking great, thank you!


.. _netcdf.group.warning:

.. warning::
Copy link
Member

Choose a reason for hiding this comment

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

We have this same issue with round-tripping Dataset objects. Unless it's particularly more pronounced for Datatree objects, I would consider consolidating the discussion, maybe in a new section like "netCDF files that Xarray cannot represent."

Comment on lines +673 to +675
.. note::
Note that perfect round-tripping should always be possible with a zarr store (:ref:`unlike for netCDF files<netcdf.group.warning>`),
as zarr does not support "unused" dimensions.
Copy link
Member

Choose a reason for hiding this comment

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

I would just leave this note under the netCDF section -- only people concerned about netCDF need to worry about this

Comment on lines +3692 to +3697
except ImportError:
raise ImportError(
"Could not import the datatree package. "
"Find it at https://github.com/xarray-contrib/datatree"
)

Copy link
Member

Choose a reason for hiding this comment

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

Python will give more informative error message ("The above exception was the direct cause of the following exception") if you use raise ... from ... here:

Suggested change
except ImportError:
raise ImportError(
"Could not import the datatree package. "
"Find it at https://github.com/xarray-contrib/datatree"
)
except ImportError as e:
raise ImportError(
"Could not import the datatree package. "
"Find it at https://github.com/xarray-contrib/datatree"
) from e

Comment on lines +6631 to +6637
try:
from datatree import DataTree

if isinstance(other, DataTree):
return NotImplemented
except ImportError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +51 to +52
# This ordering won't work unless xarray.Dataset defers to DataTree.
# See https://github.com/xarray-contrib/datatree/issues/146
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this comment can be deleted now :)

ds1 = Dataset({"a": [5], "b": [3]})
ds2 = Dataset({"x": [0.1, 0.2], "y": [10, 20]})
dt = DataTree(data=ds1)
DataTree(name="subnode", data=ds2, parent=dt)
Copy link
Member

Choose a reason for hiding this comment

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

Does this line do anything?

other_ds = Dataset({"z": ("z", [0.1, 0.2])})

expected = DataTree(data=ds1 * other_ds)
DataTree(name="subnode", data=ds2 * other_ds, parent=expected)
Copy link
Member

Choose a reason for hiding this comment

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

Same concern as above

@TomNicholas
Copy link
Member Author

This was superceded by #9619

@TomNicholas TomNicholas deleted the datatree_commutativity branch October 17, 2024 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tools dependencies Pull requests that update a dependency file topic-DataTree Related to the implementation of a DataTree class topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants