DataTree.lineage should be renamed to .parents #286

etienneschalk · 2023-12-03T00:48:30Z

Open Points

(1) Should a deprecation warning be added? Currently, no deprecation warning.
- Yes
(2) Should an sub-issue be created from Consistency between DataTree methods and pathlib.PurePath methods #283, or is the task small enough that it can leave in a Pull Request?
- No
(3) Should lineage and iter_lineage be removed from api.rst?
- No
- Reason: See DataTree.lineage should be renamed to .parents #286 (comment)

Checklist

Subtask DataTree.lineage should be renamed to .parents of Consistency between DataTree methods and pathlib.PurePath methods #283
Tests added
- Renamed test_lineage to test_parents
- Rewrote test_lineage so that it tests that the lineage and parents properties are equivalent
- Wrote test_iter_lineage to verify that iter_lineage and iter_parents yield the same results
Passes pre-commit run --all-files
- ⚠️ I had to add a #type: ignore comment
New functions/methods are listed in api.rst
- lineage and iter_lineage removed (see open point (3))
- parents and iter_parents added
Changes are summarized in docs/source/whats-new.rst
- In section "Deprecation"

TomNicholas · 2023-12-03T01:10:05Z

Thanks for this! I will give a proper review tomorrow!

Should a deprecation warning be added?

Technically we should add the new method, alias the old method to point to the new, and raise a DeprecationWarning whenever the old method is used.

Does this need it's own issue?

No, we'll just tick off that bullet point when this done.

etienneschalk · 2023-12-03T14:17:04Z

Technically we should add the new method, alias the old method to point to the new, and raise a DeprecationWarning whenever the old method is used.

Okay, I only substituted lineage by parents. I will rework the PR.

I can get inspiration from other places where warnings (DeprecationWarning) are raised, and the xarray contributing guidelines also underline the importance of not breaking the API : Backwards Compatibility

TomNicholas

This is looking good so far! Just one comment about iter_parents.

docs/source/whats-new.rst

datatree/treenode.py

datatree/tests/test_treenode.py

datatree/mapping.py

TomNicholas · 2023-12-03T18:13:27Z

Should lineage and iter_lineage be removed from api.rst?

I actually can't remember how we normally do this, but I think probably better to leave them in and only remove them from api.rst once they are actually removed from the library.

I can get inspiration from other places where warnings (DeprecationWarning) are raised, and the xarray contributing guidelines also underline the importance of not breaking the API : Backwards Compatibility

It's great that you found this! And whilst broadly we should try to follow that, in datatree we do not need to be as strict about it as we are in xarray (see https://github.com/xarray-contrib/datatree#development-roadmap).

etienneschalk · 2023-12-03T18:52:21Z

So, I added back lineage in api.rst and my credit, it is ready for review!

TomNicholas · 2023-12-03T22:14:16Z

datatree/treenode.py

@@ -236,25 +236,48 @@ 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]:


Sorry I don't think I was clear. I think we should never add .iter_parents, instead recommending the new .parents as the replacement for both .lineage and .iter_lineage. So there should not be any method called iter_parents.

etienneschalk · 2023-12-04T22:46:04Z

datatree/tests/test_treenode.py

@@ -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)


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

Note:lineage is still tested in test_lineage

etienneschalk · 2023-12-04T22:48:03Z

datatree/treenode.py

@@ -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]:


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

etienneschalk · 2023-12-04T22:48:33Z

datatree/treenode.py

+            "Please use `parents` from now on.",
+            DeprecationWarning,
+        )
+        return self.iter_lineage()


Call chain: lineage -> iter_lineage -> parents

etienneschalk · 2023-12-04T22:52:52Z

datatree/treenode.py

@@ -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)):


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

etienneschalk · 2023-12-04T22:54:57Z

datatree/treenode.py

-        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 "."


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:

datatree/datatree/tests/test_datatree.py

Line 98 in 66397e8

result = sue.relative_to(john)

TomNicholas · 2023-12-09T21:04:44Z

I don't know why I keep having to re-approve the automated tests on this PR ☹️

This is looking great @etienneschalk ! Tag me when you feel it's ready for another review!

etienneschalk · 2023-12-09T21:38:54Z

Hello @TomNicholas ,

It seems that the CI is not automatically triggered on push and required manual intervention from a maintainer.

I assume that having passing CI checks is a pre-condition to make a new review from your side worth it.

What actions can I do on my side to fix these tests? I see some are failed or cancelled.

Locally, what I do currently before pushing is:

Run pytest
Make sure pre-commit pass

However, I don't:

Build the docs (I cannot currently, Doc build error #284 (comment) )
Test other python versions (I am using a dev environment with Python 3.10.12)

TomNicholas · 2023-12-09T22:01:33Z

It seems that the CI is not automatically triggered on push and required manual intervention from a maintainer.

Yeah... I'm trying to work out what's causing that but drawing a blank so far.

What actions can I do on my side to fix these tests? I see some are failed or cancelled.

I think we have another issue caused by using the most recent version of xarray, see #270.

Co-authored-by: Tom Nicholas <[email protected]>

etienneschalk · 2024-01-06T16:40:23Z

This PR should be ready to be merged, after #298 has been merged (to fix #284)

TomNicholas · 2024-01-19T22:39:40Z

Thanks so much @etienneschalk ! Sorry it took me a while to look at this ❤️

TomNicholas suggested changes Dec 3, 2023

View reviewed changes

docs/source/whats-new.rst Show resolved Hide resolved

datatree/treenode.py Outdated Show resolved Hide resolved

datatree/tests/test_treenode.py Outdated Show resolved Hide resolved

TomNicholas reviewed Dec 3, 2023

View reviewed changes

datatree/mapping.py Outdated Show resolved Hide resolved

TomNicholas reviewed Dec 3, 2023

View reviewed changes

TomNicholas mentioned this pull request Dec 3, 2023

Should the parent of the root node be None or itself? #288

Closed

etienneschalk commented Dec 4, 2023

View reviewed changes

etienneschalk force-pushed the eschalk/rename-lineage-to-parents branch from dcf24fd to 2c0b3ea Compare December 9, 2023 20:22

etienneschalk and others added 14 commits January 6, 2024 16:32

Replace 'lineage' occurences in code by 'parents'

ec5457a

Replace 'lineage' occurences in api.rst by 'parents'

cdf5ad3

MyPy ignore

423e68a

whats-new

9a91490

Re-introduce lineage and deprecate it

52bc31b

Added credit

543402c

Added back lineage in api.rst

2d465d8

Update datatree/tests/test_treenode.py

91854bc

Co-authored-by: Tom Nicholas <[email protected]>

Updated lineage and parents, broke tests

4cf5130

Replaced slash by point, tests pass

d3ea0db

New PR

a1cdfb4

Fix tests

1e45e3d

Remove iter_parents from api.rst

057a638

Avoid entering into the more complex else branch

6d1d787

etienneschalk force-pushed the eschalk/rename-lineage-to-parents branch from 2c0b3ea to 6d1d787 Compare January 6, 2024 15:38

etienneschalk mentioned this pull request Jan 6, 2024

Use napoleon instead of numpydoc (xarray doc alignment), and fixes #298

Merged

3 tasks

Merge branch 'main' into eschalk/rename-lineage-to-parents

fbdc558

etienneschalk requested a review from TomNicholas January 9, 2024 20:35

Merge branch 'main' into eschalk/rename-lineage-to-parents

6375923

TomNicholas enabled auto-merge (squash) January 19, 2024 22:35

TomNicholas merged commit 8fbf2be into xarray-contrib:main Jan 19, 2024
14 checks passed

etienneschalk deleted the eschalk/rename-lineage-to-parents branch January 20, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataTree.lineage should be renamed to .parents #286

DataTree.lineage should be renamed to .parents #286

etienneschalk commented Dec 3, 2023 •

edited

Loading

TomNicholas commented Dec 3, 2023

etienneschalk commented Dec 3, 2023

TomNicholas left a comment

TomNicholas commented Dec 3, 2023

etienneschalk commented Dec 3, 2023

TomNicholas Dec 3, 2023

etienneschalk Dec 4, 2023

etienneschalk Dec 4, 2023

etienneschalk Dec 4, 2023

etienneschalk Dec 4, 2023

etienneschalk Dec 4, 2023

TomNicholas commented Dec 9, 2023

etienneschalk commented Dec 9, 2023

TomNicholas commented Dec 9, 2023

etienneschalk commented Jan 6, 2024 •

edited

Loading

TomNicholas commented Jan 19, 2024

DataTree.lineage should be renamed to .parents #286

DataTree.lineage should be renamed to .parents #286

Conversation

etienneschalk commented Dec 3, 2023 • edited Loading

Open Points

Checklist

TomNicholas commented Dec 3, 2023

etienneschalk commented Dec 3, 2023

TomNicholas left a comment

Choose a reason for hiding this comment

TomNicholas commented Dec 3, 2023

etienneschalk commented Dec 3, 2023

TomNicholas Dec 3, 2023

Choose a reason for hiding this comment

etienneschalk Dec 4, 2023

Choose a reason for hiding this comment

etienneschalk Dec 4, 2023

Choose a reason for hiding this comment

etienneschalk Dec 4, 2023

Choose a reason for hiding this comment

etienneschalk Dec 4, 2023

Choose a reason for hiding this comment

etienneschalk Dec 4, 2023

Choose a reason for hiding this comment

TomNicholas commented Dec 9, 2023

etienneschalk commented Dec 9, 2023

TomNicholas commented Dec 9, 2023

etienneschalk commented Jan 6, 2024 • edited Loading

TomNicholas commented Jan 19, 2024

etienneschalk commented Dec 3, 2023 •

edited

Loading

etienneschalk commented Jan 6, 2024 •

edited

Loading