-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Migrate treenode module. #8757
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
Migrate treenode module. #8757
Changes from 13 commits
065db0e
f167b99
6bd492f
b62a21a
32053b6
32e7453
7f2a178
b4fb773
548536b
b9685d7
59da654
b821fca
4530973
c03d373
ccd5374
4830cd4
e8459bc
4238ce2
db264c0
b4acb0d
0f4b38a
e7ea2b4
f2f327f
9cbaf3b
9db7040
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,16 +2,12 @@ | |
|
||
import sys | ||
from collections import OrderedDict | ||
from collections.abc import Iterator, Mapping | ||
from pathlib import PurePosixPath | ||
from typing import ( | ||
TYPE_CHECKING, | ||
Generic, | ||
Iterator, | ||
Mapping, | ||
Optional, | ||
Tuple, | ||
TypeVar, | ||
Union, | ||
) | ||
|
||
from xarray.core.utils import Frozen, is_dict_like | ||
|
@@ -25,7 +21,7 @@ class InvalidTreeError(Exception): | |
|
||
|
||
class NotFoundInTreeError(ValueError): | ||
"""Raised when operation can't be completed because one node is part of the expected tree.""" | ||
"""Raised when operation can't be completed because one node is not part of the expected tree.""" | ||
|
||
|
||
class NodePath(PurePosixPath): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're exposing it (and keep it as a pure path, which I think we should), should we move this class to a separate module? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we exposing What's the logic for making it a pure path? The posix path is to ensure the slashes are in a consistent direction (remember this isn't a real filepath so shouldn't change automatically on windows systems). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure about this, but I believe the difference is that The main difference between filesystems and On the other hand, a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I wrote my above comment too quickly - we both agree it should stay as a pure path. We could also consider adding extra methods to this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related issue: xarray-contrib/datatree#283 (Consistency between DataTree methods and pathlib.PurePath methods) To me the graal would be to have datatree more and more compatible with As @keewis mentioned , there is a single instance of the filesystem whereas there can be multiple DataTrees. So a single DataTree could act like a "small filesystem on its own" and implement the
The idea here is to delegate to Concrete example: working with multi-resolution rasters (a usecase of datatree). By using f-strings and glob, you can write resolution independant code once, like you would have done by manipulating a data structure on the file system The schema on the pathlib doc (arrows are inheritance): flowchart BT
PurePosixPath --> PurePath
Path --> PurePath
PureWindowsPath --> PurePath
PosixPath --> PurePosixPath
PosixPath --> Path
WindowsPath --> PureWindowsPath
WindowsPath --> Path
How I see it: flowchart BT
subgraph Pure
PurePosixPath --> PurePath
NodePath --> PurePosixPath
end
subgraph Concrete
DataTree --> NodePath
PosixPath --> PurePosixPath
end
(I removed I am not 100% sure it fully makes sense... The nuance is that when you instanciate a Path, it is always implicit that it is bound to the filesystem, while each DataTree instantiation creates its own data representation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @etienneschalk for all your thoughts here!
I strongly feel that DataTree / NodePath objects should not be associated with concrete filesystem paths. However this
seems like a reasonable idea, and I think we should continue the discussion of that in xarray-contrib/datatree#283. For now I don't think there is anything we need to do in this PR specifically - we can revisit the API choices / accepting path-like types in a future PR. |
||
|
@@ -75,10 +71,10 @@ class TreeNode(Generic[Tree]): | |
(This class is heavily inspired by the anytree library's NodeMixin class.) | ||
""" | ||
|
||
_parent: Optional[Tree] | ||
_parent: Tree | None | ||
_children: OrderedDict[str, Tree] | ||
|
||
def __init__(self, children: Optional[Mapping[str, Tree]] = None): | ||
def __init__(self, children: Mapping[str, Tree] | None = None): | ||
"""Create a parentless node.""" | ||
self._parent = None | ||
self._children = OrderedDict() | ||
|
@@ -91,7 +87,7 @@ def parent(self) -> Tree | None: | |
return self._parent | ||
|
||
def _set_parent( | ||
self, new_parent: Tree | None, child_name: Optional[str] = None | ||
self, new_parent: Tree | None, child_name: str | None = None | ||
) -> None: | ||
# TODO is it possible to refactor in a way that removes this private method? | ||
|
||
|
@@ -137,7 +133,7 @@ def _detach(self, parent: Tree | None) -> None: | |
self._parent = None | ||
self._post_detach(parent) | ||
|
||
def _attach(self, parent: Tree | None, child_name: Optional[str] = None) -> None: | ||
def _attach(self, parent: Tree | None, child_name: str | None = None) -> None: | ||
if parent is not None: | ||
if child_name is None: | ||
raise ValueError( | ||
|
@@ -242,7 +238,7 @@ def _iter_parents(self: Tree) -> Iterator[Tree]: | |
yield node | ||
node = node.parent | ||
|
||
def iter_lineage(self: Tree) -> Tuple[Tree, ...]: | ||
def iter_lineage(self: Tree) -> tuple[Tree, ...]: | ||
"""Iterate up the tree, starting from the current node.""" | ||
from warnings import warn | ||
|
||
|
@@ -254,7 +250,7 @@ def iter_lineage(self: Tree) -> Tuple[Tree, ...]: | |
return tuple((self, *self.parents)) | ||
|
||
@property | ||
def lineage(self: Tree) -> Tuple[Tree, ...]: | ||
def lineage(self: Tree) -> tuple[Tree, ...]: | ||
"""All parent nodes and their parent nodes, starting with the closest.""" | ||
from warnings import warn | ||
|
||
|
@@ -266,12 +262,12 @@ def lineage(self: Tree) -> Tuple[Tree, ...]: | |
return self.iter_lineage() | ||
|
||
@property | ||
def parents(self: Tree) -> Tuple[Tree, ...]: | ||
def parents(self: Tree) -> tuple[Tree, ...]: | ||
"""All parent nodes and their parent nodes, starting with the closest.""" | ||
return tuple(self._iter_parents()) | ||
|
||
@property | ||
def ancestors(self: Tree) -> Tuple[Tree, ...]: | ||
def ancestors(self: Tree) -> tuple[Tree, ...]: | ||
"""All parent nodes and their parent nodes, starting with the most distant.""" | ||
|
||
from warnings import warn | ||
|
@@ -306,7 +302,7 @@ def is_leaf(self) -> bool: | |
return self.children == {} | ||
|
||
@property | ||
def leaves(self: Tree) -> Tuple[Tree, ...]: | ||
def leaves(self: Tree) -> tuple[Tree, ...]: | ||
""" | ||
All leaf nodes. | ||
|
||
|
@@ -341,12 +337,12 @@ def subtree(self: Tree) -> Iterator[Tree]: | |
-------- | ||
DataTree.descendants | ||
""" | ||
from . import iterators | ||
from xarray.datatree_.datatree import iterators | ||
|
||
return iterators.PreOrderIter(self) | ||
|
||
@property | ||
def descendants(self: Tree) -> Tuple[Tree, ...]: | ||
def descendants(self: Tree) -> tuple[Tree, ...]: | ||
""" | ||
Child nodes and all their child nodes. | ||
|
||
|
@@ -431,7 +427,7 @@ def _post_attach(self: Tree, parent: Tree) -> None: | |
"""Method call after attaching to `parent`.""" | ||
pass | ||
|
||
def get(self: Tree, key: str, default: Optional[Tree] = None) -> Optional[Tree]: | ||
def get(self: Tree, key: str, default: Tree | None = None) -> Tree | None: | ||
""" | ||
Return the child node with the specified key. | ||
|
||
|
@@ -445,7 +441,7 @@ def get(self: Tree, key: str, default: Optional[Tree] = None) -> Optional[Tree]: | |
|
||
# TODO `._walk` method to be called by both `_get_item` and `_set_item` | ||
|
||
def _get_item(self: Tree, path: str | NodePath) -> Union[Tree, T_DataArray]: | ||
def _get_item(self: Tree, path: str | NodePath) -> Tree | T_DataArray: | ||
""" | ||
Returns the object lying at the given path. | ||
|
||
|
@@ -488,24 +484,26 @@ def _set(self: Tree, key: str, val: Tree) -> None: | |
def _set_item( | ||
self: Tree, | ||
path: str | NodePath, | ||
item: Union[Tree, T_DataArray], | ||
item: Tree | T_DataArray, | ||
new_nodes_along_path: bool = False, | ||
allow_overwrite: bool = True, | ||
) -> None: | ||
""" | ||
Set a new item in the tree, overwriting anything already present at that path. | ||
|
||
The given value either forms a new node of the tree or overwrites an existing item at that location. | ||
The given value either forms a new node of the tree or overwrites an | ||
existing item at that location. | ||
|
||
Parameters | ||
---------- | ||
path | ||
item | ||
new_nodes_along_path : bool | ||
If true, then if necessary new nodes will be created along the given path, until the tree can reach the | ||
specified location. | ||
If true, then if necessary new nodes will be created along the | ||
given path, until the tree can reach the specified location. | ||
allow_overwrite : bool | ||
Whether or not to overwrite any existing node at the location given by path. | ||
Whether or not to overwrite any existing node at the location given | ||
by path. | ||
|
||
Raises | ||
------ | ||
|
@@ -580,8 +578,8 @@ class NamedNode(TreeNode, Generic[Tree]): | |
Implements path-like relationships to other nodes in its tree. | ||
""" | ||
|
||
_name: Optional[str] | ||
_parent: Optional[Tree] | ||
_name: str | None | ||
_parent: Tree | None | ||
_children: OrderedDict[str, Tree] | ||
|
||
def __init__(self, name=None, children=None): | ||
|
@@ -603,8 +601,14 @@ def name(self, name: str | None) -> None: | |
raise ValueError("node names cannot contain forward slashes") | ||
self._name = name | ||
|
||
def __repr__(self, level=0): | ||
repr_value = "\t" * level + self.__str__() + "\n" | ||
for child in self.children: | ||
repr_value += self.get(child).__repr__(level + 1) | ||
return repr_value | ||
|
||
def __str__(self) -> str: | ||
return f"NamedNode({self.name})" if self.name else "NamedNode()" | ||
return f"NamedNode('{self.name}')" if self.name else "NamedNode()" | ||
|
||
def _post_attach(self: NamedNode, parent: NamedNode) -> None: | ||
"""Ensures child has name attribute corresponding to key under which it has been stored.""" | ||
|
Uh oh!
There was an error while loading. Please reload this page.