Children proxy interface #9477

TomNicholas · 2024-09-11T02:22:42Z

Just a sketch for now - follows the same general idea of .coords.

Closes Set child nodes via DataTree.children #9441
Tests added
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

TomNicholas · 2024-09-11T02:30:57Z

xarray/core/treenode.py

@@ -44,6 +45,68 @@ def __init__(self, *pathsegments):
 Tree = TypeVar("Tree", bound="TreeNode")


+class Children(Mapping[str, Tree], Generic[Tree]):
+    """
+    Dictionary-like container for the immediate children of a single DataTree node.


I'm not sure whether this class should allow path-like access to grandchildren or only allow access to immediate children.

Same question also for setting

On second thoughts I think this class should only allow getting/setting immediate children (because it's basically a glorified dict).

On second thoughts I think this class should only allow getting/setting immediate children (because it's basically a glorified dict).

Agreed!

This relates to #9485 though - why exactly do we want to support full paths for .coords but only local access for .children? We could do whatever, but what's the rationale for them being different?

So you're basically thinking of .coords/.children as meaning "access to all coords/children defined anywhere on the subtree". I originally thought of them as "access to just coords/children defined on this node".

FYI path-like access like this means that list(dt.children) will return a subset of what can be accessed via .__getitem__, (and __contains__ could go either way, see #9354)).

One compromise option of sorts (similar to what is done for Dataset for derived variables like time.year, or for coordinates in Dataset.__getitem__) is to only list immediate children in __iter__ and __contains__ but to support accessing them in __getitem__.

There is a detailed discussion about this on the old repo (xarray-contrib/datatree#240 (comment)). There the conclusion was that __contains__ should support paths if __getitem__ does, but it's okay for .keys()/.values()/.items() to be local only.

So you're basically thinking of .coords/.children as meaning "access to all coords/children defined anywhere on the subtree".

To play devil's advocate a bit: If .coords can access the whole tree then why not also .variables? I think they should all be consistent.

There is a detailed discussion about this on the old repo (xarray-contrib/datatree#240 (comment)). There the conclusion was that __contains__ should support paths if __getitem__ does, but it's okay for .keys()/.values()/.items() to be local only.

Yes, that seems to be how Dataset works for coordinates.

To play devil's advocate a bit: If .coords can access the whole tree then why not also .variables? I think they should all be consistent.

Sure, we could do that. Hopefully it's all very straightforwardly reusing the same machinery?

Yes, that seems to be how Dataset works for coordinates.

I'm not sure what you mean. You're talking about virtual vs non-virtual coordinates on Dataset? That they are all in __contains__ and __getitem__, but only a subset (the non-virtual ones) are listed by .keys()?

If .coords can access the whole tree then why not also .variables?

Sure, we could do that.

I actually meant that as a example of counter-intuitive behaviour 😅 . But maybe it's fine too? That would mean we have really done a 180 degree turn in this issue... It would imply changing the current behaviour of dt.data_vars too.

Hopefully it's all very straightforwardly reusing the same machinery?

It's not right now (local vs path-supporting have different codepaths), but it could be made to: everything could go through some version of using TreeNode._set_item

xarray/xarray/core/treenode.py

Line 550 in 8bd456c

def _set_item(

or ._walk_to and we could add a parameter like traverse_paths to generalize it for both cases. Generalizing .update to support paths might be the approach that is easiest to integrate with the Coordinates base class.

Also one final thing: We are talking here about supporting paths to descendant nodes only? Or upwards via dt['../../path/to/node/'] too?

TomNicholas · 2024-09-11T12:38:40Z

xarray/tests/test_treenode.py

@@ -224,6 +230,56 @@ def test_overwrite_child(self):
        assert marys_evil_twin.parent is john


+class TestChildren:


It's a bit awkward to know whether to put tests for this class in test_treenode.py or test_datatree.py.

On the one hand the concept of access to .children is very much part of the TreeNode structure - it has nothing to do with .data. On the other hand it's hard to properly test it without full features that are only available on DataTree, such as .copy, a nice repr, and assertions. It's also a fully public-facing class, unlike TreeNode.

This is a lot less awkward after #9482.

TomNicholas · 2024-09-11T14:48:23Z

xarray/tests/test_treenode.py

+        # test constructor
+        john2 = TreeNode(children=children)
+        assert john2.children == children


This test failure is due to #9478

shoyer · 2024-09-12T20:06:01Z

xarray/core/treenode.py

@@ -44,6 +45,68 @@ def __init__(self, *pathsegments):
 Tree = TypeVar("Tree", bound="TreeNode")


+class Children(Mapping[str, Tree], Generic[Tree]):
+    """
+    Dictionary-like container for the immediate children of a single DataTree node.


On second thoughts I think this class should only allow getting/setting immediate children (because it's basically a glorified dict).

Agreed!

shoyer · 2024-09-12T20:09:17Z

xarray/core/treenode.py

+        children = self._treenode._children.copy()
+        children.update(other)
+        self._treenode.children = children


Just to verify, this is a no-op if children have incompatible indexes, right?

The .children setter defined on DataTree (and TreeNode) will check for name collisions between variables and children, and if it fails it will avoid modification in-place (we can thank the anytree library for that bit of code ¹). In the same place then check_alignment also gets called. Is that what you meant?

Footnotes

Should we adjust the licensing part of the readme as we're bundling small portions of anytree code? We did already add it to the licenses directory here. ↩

shoyer · 2024-09-12T20:09:40Z

xarray/core/treenode.py

+        if not len(other):
+            return


It's slightly more canonical Python to avoid the call to len():

Suggested change

if not len(other):

return

if not other:

return

shoyer · 2024-09-12T20:11:15Z

xarray/core/treenode.py

+    def _names(self) -> list[str]:
+        return list(self._treenode._children.keys())


Why not directly use the set-like DictKeys object?

Suggested change

def _names(self) -> list[str]:

return list(self._treenode._children.keys())

def _names(self) -> Set[str]:

return self._treenode._children.keys()

I can probably use .keys() you're right, but I don't think I should type this as returning Set, because I originally tried return set(self._treenode._children) and that failed intermittently due to ordering differences.

I was thinking of collections.abc.Set, which is an abstract base class that includes DictKeys, not the specific built-in set class

TomNicholas added 2 commits September 10, 2024 22:20

sketch tests

320a8a2

sketch implementation

1383211

TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label Sep 11, 2024

TomNicholas marked this pull request as draft September 11, 2024 02:24

TomNicholas commented Sep 11, 2024

View reviewed changes

TomNicholas added 7 commits September 10, 2024 22:32

make ._names a property

e61112f

call private ._children

2c1b13f

draft implementation of .update

03c56ea

Merge branch 'main' into children_proxy_interface

0ca7f67

fix bug with ordering of child names

860764d

completely comment out repr test for now

51c198e

check only immediate children are accessible

e0358aa

TomNicholas commented Sep 11, 2024

View reviewed changes

TomNicholas added 2 commits September 11, 2024 09:21

add tests for modifying via .children

08f83f8

temporary repr for debugging

7f87d08

TomNicholas commented Sep 11, 2024

View reviewed changes

TomNicholas added 2 commits September 11, 2024 17:12

Merge branch 'main' into children_proxy_interface

1dace08

implement a very simple repr

d6de4a1

shoyer mentioned this pull request Sep 12, 2024

DAS-2155 - Merge datatree documentation into main docs. #9033

Merged

3 tasks

TomNicholas mentioned this pull request Sep 12, 2024

DataTree: Copy during assignment to child nodes? #9489

Open

shoyer approved these changes Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Children proxy interface #9477

Children proxy interface #9477

TomNicholas commented Sep 11, 2024

TomNicholas Sep 11, 2024

TomNicholas Sep 11, 2024

TomNicholas Sep 11, 2024 •

edited

Loading

shoyer Sep 12, 2024

TomNicholas Sep 12, 2024

TomNicholas Sep 12, 2024

shoyer Sep 12, 2024

TomNicholas Sep 12, 2024

shoyer Sep 12, 2024

TomNicholas Sep 12, 2024

TomNicholas Sep 11, 2024 •

edited

Loading

TomNicholas Sep 11, 2024

TomNicholas Sep 11, 2024

shoyer Sep 12, 2024

shoyer Sep 12, 2024

TomNicholas Sep 12, 2024

shoyer Sep 12, 2024

shoyer Sep 12, 2024

TomNicholas Sep 12, 2024

shoyer Sep 12, 2024

		@@ -224,6 +230,56 @@ def test_overwrite_child(self):
		assert marys_evil_twin.parent is john


		class TestChildren:

		def _names(self) -> list[str]:
		return list(self._treenode._children.keys())

Children proxy interface #9477

Are you sure you want to change the base?

Children proxy interface #9477

Conversation

TomNicholas commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas Sep 11, 2024 •

edited

Loading

TomNicholas Sep 11, 2024 •

edited

Loading