Implement move method for moving nodes
#225
Conversation
There was a problem hiding this comment.
Thanks for opening this @JosiasAurel ! I've added a few comments. I do wonder if the approach I suggested in this comment might be simpler though.
docs/source/whats-new.rst
Outdated
| .. _whats-new.v0.0.13: | ||
|
|
||
| v0.0.12 (03/16/2023) | ||
| -------------------- | ||
|
|
||
| New Features | ||
| ~~~~~~~~~~~~ | ||
|
|
||
| - Added a :py:func:`DataTree.move` | ||
| By `JosiasAurel <[email protected]>`_. | ||
|
|
There was a problem hiding this comment.
It doesn't look like this has been added in the correct place. It should be under the unreleased version v0.0.13
| @@ -853,6 +853,22 @@ def __setitem__( | |||
| else: | |||
| raise ValueError("Invalid format for key") | |||
|
|
|||
| def move(self, original_path: str, to: str) -> DataTree: | |||
There was a problem hiding this comment.
This method needs a docstring, following the format of other xarray methods.
datatree/datatree.py
Outdated
| @@ -853,6 +853,22 @@ def __setitem__( | |||
| else: | |||
| raise ValueError("Invalid format for key") | |||
|
|
|||
| def move(self, original_path: str, to: str) -> DataTree: | |||
| new_tree = self.copy() | |||
| target_node = new_tree.__getitem__(original_path) | |||
There was a problem hiding this comment.
Why not use the square bracket syntax here? i.e.
target_node = new_tree[original_path]
datatree/datatree.py
Outdated
| target_node = new_tree.__getitem__(original_path) | ||
| path_to_parent = "/".join(original_path.split("/")[:-1]) | ||
|
|
||
| new_tree.__setitem__(to, DataTree(target_node.to_dataset())) |
There was a problem hiding this comment.
Could also use square brackets here
datatree/datatree.py
Outdated
| def move(self, original_path: str, to: str) -> DataTree: | ||
| new_tree = self.copy() | ||
| target_node = new_tree.__getitem__(original_path) | ||
| path_to_parent = "/".join(original_path.split("/")[:-1]) |
There was a problem hiding this comment.
We have a relative_to method for this - if you look at the bottom of treenode.py there is a NodePath class you can use that will be much cleaner.
There was a problem hiding this comment.
Here I'm trying to get the path to parent. I'm really not sure how relative_to is going to help me here given that I only have the path to the node itself
There was a problem hiding this comment.
In the TreeNode Class, there is a method, _set_parent ( ) , it takes, new_parent, and, child_name, as arguments. I wonder if this would simplify our move method. Since moving the node is actually re-assigning parent. @JosiasAurel , @TomNicholas
There was a problem hiding this comment.
@TomNicholas please do you mind checking my latest changes? Thank you
|
|
||
| # if empty str then it's likely the root node | ||
| if path_to_parent == "": | ||
| new_tree = new_tree.drop_nodes(str(target_node.name)) |
There was a problem hiding this comment.
You might want to add a comment explaining why you are dropping nodes here. (I'm not sure why you are either)
| name="TestMove", | ||
| ) | ||
|
|
||
| def test_move_to_root(self): |
There was a problem hiding this comment.
This test looks useful, but it doesn't actually move anything to the root position like the name suggests.
| @@ -440,6 +440,38 @@ def test_setitem_dataarray_replace_existing_node(self): | |||
| xrt.assert_identical(results.to_dataset(), expected) | |||
|
|
|||
|
|
|||
| class TestMoveNode: | |||
There was a problem hiding this comment.
Great that you added tests! That's important.
datatree/tests/test_datatree.py
Outdated
| xrt.assert_equal( | ||
| self.dt["/Jake"].to_dataset(), new_dt["/Bragg/Kyle/Jake"].to_dataset() | ||
| ) |
There was a problem hiding this comment.
There are assert functions for comparing two datatrees that you can use to check that the whole tree is as expected.
Generally you want tests to take the form
def test_thing():
original = ...
expected_result = ...
result = dt.thing()
assert_datatree_equal(result, expected_result)
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
pre-commit run --all-filesapi.rstdocs/source/whats-new.rst