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

DataTree.lineage should be renamed to .parents #286

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,5 @@ dmypy.json
# version
_version.py

.vscode
# Ignore vscode specific settings
.vscode/
11 changes: 7 additions & 4 deletions datatree/tests/test_treenode.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def test_ancestors(self):
michael = TreeNode(children={"Tony": tony})
vito = TreeNode(children={"Michael": michael})
assert tony.root is vito
assert tony.lineage == (tony, michael, vito)
assert tony.parents == (michael, vito)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A node's parents do not include the node itself, in opposition to lineage.

Note:lineage is still tested in test_lineage

assert tony.ancestors == (vito, michael, tony)


Expand Down Expand Up @@ -279,12 +279,15 @@ def test_levelorderiter(self):


class TestAncestry:
def test_parents(self):
_, leaf = create_test_tree()
expected = ["e", "b", "a"]
assert [node.name for node in leaf.parents] == expected

def test_lineage(self):
_, leaf = create_test_tree()
lineage = leaf.lineage
expected = ["f", "e", "b", "a"]
for node, expected_name in zip(lineage, expected):
assert node.name == expected_name
assert [node.name for node in leaf.lineage] == expected

def test_ancestors(self):
_, leaf = create_test_tree()
Expand Down
81 changes: 53 additions & 28 deletions datatree/treenode.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,7 @@ def _check_loop(self, new_parent: Tree | None) -> None:
)

def _is_descendant_of(self, node: Tree) -> bool:
_self, *lineage = list(node.lineage)
return any(n is self for n in lineage)
return any(n is self for n in node.parents)

def _detach(self, parent: Tree | None) -> None:
if parent is not None:
Expand Down Expand Up @@ -236,26 +235,53 @@ def _post_attach_children(self: Tree, children: Mapping[str, Tree]) -> None:
"""Method call after attaching `children`."""
pass

def iter_lineage(self: Tree) -> Iterator[Tree]:
def _iter_parents(self: Tree) -> Iterator[Tree]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the private _iter_parents method as it is useful to insulate the "low-level" logic (while + yield) from the parents method that just have to consume the iterator into a tuple. I could not find a way of not keeping this intermediate private method that would have been more elegant

"""Iterate up the tree, starting from the current node."""
node: Tree | None = self
node: Tree | None = self.parent
while node is not None:
yield node
node = node.parent

def iter_lineage(self: Tree) -> Tuple[Tree, ...]:
"""Iterate up the tree, starting from the current node."""
from warnings import warn

warn(
"`iter_lineage` has been deprecated, and in the future will raise an error."
"Please use `parents` from now on.",
DeprecationWarning,
)
return tuple((self, *self.parents))

@property
def lineage(self: Tree) -> Tuple[Tree, ...]:
"""All parent nodes and their parent nodes, starting with the closest."""
return tuple(self.iter_lineage())
from warnings import warn

warn(
"`lineage` has been deprecated, and in the future will raise an error."
"Please use `parents` from now on.",
DeprecationWarning,
)
return self.iter_lineage()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Call chain: lineage -> iter_lineage -> parents


@property
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, ...]:
"""All parent nodes and their parent nodes, starting with the most distant."""
if self.parent is None:
return (self,)
else:
ancestors = tuple(reversed(list(self.lineage)))
return ancestors

from warnings import warn

warn(
"`ancestors` has been deprecated, and in the future will raise an error."
"Please use `parents`. Example: `tuple(reversed(node.parents))`",
DeprecationWarning,
)
return tuple((*reversed(self.parents), self))

@property
def root(self: Tree) -> Tree:
Expand Down Expand Up @@ -351,7 +377,7 @@ def level(self: Tree) -> int:
depth
width
"""
return len(self.ancestors) - 1
return len(self.parents)

@property
def depth(self: Tree) -> int:
Expand Down Expand Up @@ -591,9 +617,9 @@ def path(self) -> str:
if self.is_root:
return "/"
else:
root, *ancestors = self.ancestors
root, *ancestors = tuple(reversed(self.parents))
# don't include name of root because (a) root might not have a name & (b) we want path relative to root.
names = [node.name for node in ancestors]
names = [*(node.name for node in ancestors), self.name]
return "/" + "/".join(names)

def relative_to(self: NamedNode, other: NamedNode) -> str:
Expand All @@ -608,7 +634,7 @@ def relative_to(self: NamedNode, other: NamedNode) -> str:
)

this_path = NodePath(self.path)
if other.path in list(ancestor.path for ancestor in self.lineage):
if other.path in list(parent.path for parent in (self, *self.parents)):
return str(this_path.relative_to(other.path))
else:
common_ancestor = self.find_common_ancestor(other)
Expand All @@ -623,18 +649,17 @@ def find_common_ancestor(self, other: NamedNode) -> NamedNode:

Raise ValueError if they are not in the same tree.
"""
common_ancestor = None
for node in other.iter_lineage():
if node.path in [ancestor.path for ancestor in self.ancestors]:
common_ancestor = node
break
if self is other:
return self

if not common_ancestor:
raise NotFoundInTreeError(
"Cannot find common ancestor because nodes do not lie within the same tree"
)
other_paths = [op.path for op in other.parents]
for parent in (self, *self.parents):
if parent.path in other_paths:
return parent

return common_ancestor
raise NotFoundInTreeError(
"Cannot find common ancestor because nodes do not lie within the same tree"
)

def _path_to_ancestor(self, ancestor: NamedNode) -> NodePath:
"""Return the relative path from this node to the given ancestor node"""
Expand All @@ -643,12 +668,12 @@ def _path_to_ancestor(self, ancestor: NamedNode) -> NodePath:
raise NotFoundInTreeError(
"Cannot find relative path to ancestor because nodes do not lie within the same tree"
)
if ancestor.path not in list(a.path for a in self.ancestors):
if ancestor.path not in list(a.path for a in (self, *self.parents)):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if the project has some test coverage tool, but I'm pretty sure this change is actually useless, as the case of the node being compared has been handled previously in the if branch of relative_to. So iterating on the parents, not including the node itself, might be correct

raise NotFoundInTreeError(
"Cannot find relative path to ancestor because given node is not an ancestor of this node"
)

lineage_paths = list(ancestor.path for ancestor in self.lineage)
generation_gap = list(lineage_paths).index(ancestor.path)
path_upwards = "../" * generation_gap if generation_gap > 0 else "/"
parents_paths = list(parent.path for parent in (self, *self.parents))
generation_gap = list(parents_paths).index(ancestor.path)
path_upwards = "../" * generation_gap if generation_gap > 0 else "."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, this path was causing me issues, but the else branch might never be reached, for the same reason. However, using a dot fixed some tests during development. It makes sense: the path from a node itself is not the root (/) but . as declared in existing test_relative_paths:

result = sue.relative_to(john)

return NodePath(path_upwards)
1 change: 1 addition & 0 deletions docs/source/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ Attributes relating to the recursive tree-like structure of a ``DataTree``.
DataTree.descendants
DataTree.siblings
DataTree.lineage
DataTree.parents
DataTree.ancestors
DataTree.groups

Expand Down
6 changes: 6 additions & 0 deletions docs/source/whats-new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ New Features
Breaking changes
~~~~~~~~~~~~~~~~

- Renamed `DataTree.lineage` to `DataTree.parents` to match `pathlib` vocabulary
(:issue:`283`, :pull:`286`)
- Minimum required version of xarray is now 2023.12.0, i.e. the latest version.
This is required to prevent recent changes to xarray's internals from breaking datatree.
(:issue:`293`, :pull:`294`)
Expand All @@ -37,6 +39,10 @@ Breaking changes
Deprecations
~~~~~~~~~~~~

- Renamed `DataTree.lineage` to `DataTree.parents` to match `pathlib` vocabulary
(:issue:`283`, :pull:`286`). `lineage` is now deprecated and use of `parents` is encouraged.
By `Etienne Schalk <https://github.com/etienneschalk>`_.

Bug fixes
~~~~~~~~~
- Keep attributes on nodes containing no data in :py:func:`map_over_subtree`. (:issue:`278`, :pull:`279`)
Expand Down
Loading