-
Notifications
You must be signed in to change notification settings - Fork 41
Out of node access to variables in .ds #102
Conversation
This reverts commit 52ef23b.
datatree/tests/test_datatree.py
Outdated
def test_getitem(self): | ||
dt = create_test_datatree() | ||
da = dt.ds["a"] | ||
print(da) | ||
expected = xr.DataArray(dims=("y",), data=[6, 7, 8]) | ||
print(expected) | ||
xrt.assert_equal(da, expected) | ||
|
||
# @pytest.mark.xfail | ||
def test_get_items_in_other_nodes(self): | ||
... | ||
dt = create_test_datatree() | ||
node_dsv = dt["set1"].ds | ||
|
||
xrt.assert_identical(node_dsv["../a"], dt["a"]) | ||
|
||
# don't allow retrieving other DataTree nodes | ||
with pytest.raises(KeyError): | ||
node_dsv["../set2"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling .ds.__getitem__
anywhere in these tests currently raises a RecursionError
, and the cause is something complicated to do with xarray.Dataset
's __getattr__
manipulation interacting with me adding a new _wrapping_node
attribute to my DatasetView
subclass.
dt = create_test_datatree() | ||
node_dsv = dt["set1"].ds | ||
|
||
xrt.assert_identical(node_dsv["../a"], dt["a"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to #99 (comment)
When users pull out a dataset from a datatree, how do they know that it has access to "../a"
. Is it shown in the representation and/or are you planning to change the __repr__
of Dataset?
cc: @aurghs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly hadn't really thought of that - with this implementation they wouldn't know without looking at documentation, which is not ideal.
Based on discussion in #99 (comment) we will leave this for now, and see if it's actually something that users particularly want later. |
Closing this as probably not a good idea. |
Intended to close part (2) of #80, but currently raises a RecursionError.
pre-commit run --all-files
api.rst
docs/source/whats-new.rst