Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

Don't expose properties of wrapped dataset? #36

Closed
TomNicholas opened this issue Sep 2, 2021 · 1 comment · Fixed by #37
Closed

Don't expose properties of wrapped dataset? #36

TomNicholas opened this issue Sep 2, 2021 · 1 comment · Fixed by #37

Comments

@TomNicholas
Copy link
Member

Currently I have exposed the properties of the dataset which a node (optionally) wraps, so that a user can do

dt = DataNode(data=xr.Dataset())
dt.dims  # returns .dims property of underlying dataset

However as @jbusecke pointed out this behaviour is inconsistent with the way that all other dataset methods are dispatched over all datasets in the tree - this forwarded property only looks at the Dataset in the node it was called on, ignoring all child nodes.

I see 4 ways to think about this:

  1. Keep it as is. This would allow access to the properties without having to go via .ds first, but is also possibly counterintuitive.
  2. Return the value of the property for all nodes in the subtree. You might imagine returning something like a nested dictionary of the properties for every node in the tree. I can't really see a use case for this though, and the user can achieve something similar simply via
    dims_on_all_nodes = {node.pathstr: node.ds.dims for node in dt.subtree}
  3. Don't expose these properties at all. Then the user would have to call dt.ds.dims to access the properties. This would reinforce the distinction between a DataTree and a Dataset, even though you could still call a lot of the dataset API on the datatree object (e.g. dt.mean().) (This would also make [WIP] Automate adding Dataset properties #17 obsolete.)
  4. Make other changes that decide this issue (i.e. make DataTree inherit directly from Dataset, as discussed in Store variables in DataTree instead of storing Dataset #2 ). This would effectively be the same API as (1) though, just with automatic inheritance of the properties instead of manual wrapping of them.

I'm now leaning quite heavily towards (3).

@dcherian
Copy link

dcherian commented Sep 2, 2021

👍 for 3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants