From 5f9c13199da4220e6013b299ea067b7f83074f23 Mon Sep 17 00:00:00 2001 From: eschalk Date: Thu, 8 Feb 2024 22:06:38 +0100 Subject: [PATCH 1/8] Setting node name keeps tree linkage --- datatree/tests/test_datatree.py | 17 +++++++++++++++++ datatree/treenode.py | 7 +++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/datatree/tests/test_datatree.py b/datatree/tests/test_datatree.py index e9f373d7..437fb2b7 100644 --- a/datatree/tests/test_datatree.py +++ b/datatree/tests/test_datatree.py @@ -68,6 +68,23 @@ def test_child_gets_named_on_attach(self): mary = DataTree(children={"Sue": sue}) # noqa assert sue.name == "Sue" + def test_setting_node_name_keeps_tree_linkage(self): + root = DataTree(name="root") + child = DataTree(name="child", parent=root) + grandchild = DataTree(name="grandchild", parent=child) + + assert root.name == "root" + assert child.name == "child" + assert grandchild.name == "grandchild" + + # changing the name of a child node should correctly update the dict key in + # its parent's children + child.name = "childish" + + assert child.name == "childish" + assert "childish" in root + assert list(root.children) == ["childish"] + class TestPaths: def test_path_property(self): diff --git a/datatree/treenode.py b/datatree/treenode.py index 1689d261..66699a0d 100644 --- a/datatree/treenode.py +++ b/datatree/treenode.py @@ -91,7 +91,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? @@ -601,7 +601,10 @@ def name(self, name: str | None) -> None: raise TypeError("node name must be a string or None") if "/" in name: raise ValueError("node names cannot contain forward slashes") + parent = self._parent + self.orphan() self._name = name + self._set_parent(parent, name) def __str__(self) -> str: return f"NamedNode({self.name})" if self.name else "NamedNode()" @@ -609,7 +612,7 @@ def __str__(self) -> str: def _post_attach(self: NamedNode, parent: NamedNode) -> None: """Ensures child has name attribute corresponding to key under which it has been stored.""" key = next(k for k, v in parent.children.items() if v is self) - self.name = key + self._name = key @property def path(self) -> str: From 5a1ac1b250ef4cc44206267f9fe37d06a2b8dcb7 Mon Sep 17 00:00:00 2001 From: eschalk Date: Thu, 8 Feb 2024 22:10:23 +0100 Subject: [PATCH 2/8] what's new --- docs/source/whats-new.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/source/whats-new.rst b/docs/source/whats-new.rst index 2f6e4f88..3f5957ee 100644 --- a/docs/source/whats-new.rst +++ b/docs/source/whats-new.rst @@ -45,6 +45,10 @@ Deprecations Bug fixes ~~~~~~~~~ + +- Renaming a node also update the key of its parent's children + (:issue:`309`) + By `Etienne Schalk `_. - Keep attributes on nodes containing no data in :py:func:`map_over_subtree`. (:issue:`278`, :pull:`279`) By `Sam Levang `_. From ed0b497bfd03364a7b93981499e5e592a99633a5 Mon Sep 17 00:00:00 2001 From: eschalk Date: Thu, 8 Feb 2024 22:14:50 +0100 Subject: [PATCH 3/8] Added pull to what's new --- docs/source/whats-new.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/whats-new.rst b/docs/source/whats-new.rst index 3f5957ee..c60d25e9 100644 --- a/docs/source/whats-new.rst +++ b/docs/source/whats-new.rst @@ -47,7 +47,7 @@ Bug fixes ~~~~~~~~~ - Renaming a node also update the key of its parent's children - (:issue:`309`) + (:issue:`309`, :pull:`310`) By `Etienne Schalk `_. - Keep attributes on nodes containing no data in :py:func:`map_over_subtree`. (:issue:`278`, :pull:`279`) By `Sam Levang `_. From 3d8f4c33dc172c0646a3529ddcf0501dd576744f Mon Sep 17 00:00:00 2001 From: eschalk Date: Thu, 8 Feb 2024 23:39:12 +0100 Subject: [PATCH 4/8] Use children assignment to preserved order --- datatree/datatree.py | 2 +- datatree/tests/test_datatree.py | 24 ++++++++++++++++++++++++ datatree/treenode.py | 18 +++++++++++++++--- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/datatree/datatree.py b/datatree/datatree.py index c86c2e2e..c3f5c282 100644 --- a/datatree/datatree.py +++ b/datatree/datatree.py @@ -1482,7 +1482,7 @@ def to_netcdf( Note that unlimited_dims may also be set via ``dataset.encoding["unlimited_dims"]``. kwargs : - Addional keyword arguments to be passed to ``xarray.Dataset.to_netcdf`` + Additonal keyword arguments to be passed to ``xarray.Dataset.to_netcdf`` """ from .io import _datatree_to_netcdf diff --git a/datatree/tests/test_datatree.py b/datatree/tests/test_datatree.py index 437fb2b7..2c775b7c 100644 --- a/datatree/tests/test_datatree.py +++ b/datatree/tests/test_datatree.py @@ -85,6 +85,30 @@ def test_setting_node_name_keeps_tree_linkage(self): assert "childish" in root assert list(root.children) == ["childish"] + # changing name of root + root.name = "ginger" + + assert root.name == "ginger" + + def test_setting_node_name_keeps_tree_linkage_and_order(self): + root = DataTree.from_dict( + { + "/one": None, + "/two": None, + "/three": None, + } + ) + assert list(root.children) == ["one", "two", "three"] + + second_child = root["/two"] + second_child.name = "second" + + assert second_child.name == "second" + assert "second" in root + + # Order was preserved + assert list(root.children) == ["one", "second", "three"] + class TestPaths: def test_path_property(self): diff --git a/datatree/treenode.py b/datatree/treenode.py index 66699a0d..62f42fc9 100644 --- a/datatree/treenode.py +++ b/datatree/treenode.py @@ -601,10 +601,22 @@ def name(self, name: str | None) -> None: raise TypeError("node name must be a string or None") if "/" in name: raise ValueError("node names cannot contain forward slashes") - parent = self._parent - self.orphan() + + parent = self._parent + if parent is not None: + old_name = self._name + if old_name is None: + raise ValueError("An unnamed node should not have had a parent") + + # To preserve order of children, iterate in the ordered dict + parent.children = OrderedDict( + ( + (k if k != old_name else name, v) + for k, v in parent.children.items() + ) + ) + self._name = name - self._set_parent(parent, name) def __str__(self) -> str: return f"NamedNode({self.name})" if self.name else "NamedNode()" From dccd594880ae864c84de18a63dce75719c63c73b Mon Sep 17 00:00:00 2001 From: eschalk Date: Thu, 8 Feb 2024 23:49:38 +0100 Subject: [PATCH 5/8] fast path none --- datatree/treenode.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/datatree/treenode.py b/datatree/treenode.py index 62f42fc9..5ccb7a44 100644 --- a/datatree/treenode.py +++ b/datatree/treenode.py @@ -596,25 +596,25 @@ def name(self) -> str | None: @name.setter def name(self, name: str | None) -> None: - if name is not None: - if not isinstance(name, str): - raise TypeError("node name must be a string or None") - if "/" in name: - raise ValueError("node names cannot contain forward slashes") - - parent = self._parent - if parent is not None: - old_name = self._name - if old_name is None: - raise ValueError("An unnamed node should not have had a parent") - - # To preserve order of children, iterate in the ordered dict - parent.children = OrderedDict( - ( - (k if k != old_name else name, v) - for k, v in parent.children.items() - ) - ) + if name is None: + self._name = None + return + + if not isinstance(name, str): + raise TypeError("node name must be a string or None") + if "/" in name: + raise ValueError("node names cannot contain forward slashes") + + parent = self._parent + if parent is not None: + old_name = self._name + if old_name is None: + raise ValueError("An unnamed node should not have had a parent") + + # Iterate through the ordered dict to preserve the order of children + parent.children = OrderedDict( + ((k if k != old_name else name, v) for k, v in parent.children.items()) + ) self._name = name From 9a5d93b874ea9ed5ce14109818d57c716f0cfc17 Mon Sep 17 00:00:00 2001 From: eschalk Date: Thu, 8 Feb 2024 23:55:12 +0100 Subject: [PATCH 6/8] Vertical code --- datatree/treenode.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/datatree/treenode.py b/datatree/treenode.py index 5ccb7a44..c430a8b8 100644 --- a/datatree/treenode.py +++ b/datatree/treenode.py @@ -606,17 +606,18 @@ def name(self, name: str | None) -> None: raise ValueError("node names cannot contain forward slashes") parent = self._parent - if parent is not None: - old_name = self._name - if old_name is None: - raise ValueError("An unnamed node should not have had a parent") + if parent is None: + self._name = name + return - # Iterate through the ordered dict to preserve the order of children - parent.children = OrderedDict( - ((k if k != old_name else name, v) for k, v in parent.children.items()) - ) + old_name = self._name + if old_name is None: + raise ValueError("An unnamed node should not have had a parent") - self._name = name + # Iterate through the ordered dict to preserve the order of children + parent.children = OrderedDict( + ((k if k != old_name else name, v) for k, v in parent.children.items()) + ) def __str__(self) -> str: return f"NamedNode({self.name})" if self.name else "NamedNode()" From bd3cb24fe305399660f462871f2f34092d240dcd Mon Sep 17 00:00:00 2001 From: eschalk Date: Sat, 17 Feb 2024 12:40:46 +0100 Subject: [PATCH 7/8] Forbid node renaming to None --- datatree/datatree.py | 2 +- datatree/tests/test_datatree.py | 10 ++++++++++ datatree/treenode.py | 34 +++++++++++++++++---------------- docs/source/whats-new.rst | 2 +- 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/datatree/datatree.py b/datatree/datatree.py index c3f5c282..fa84e03e 100644 --- a/datatree/datatree.py +++ b/datatree/datatree.py @@ -1482,7 +1482,7 @@ def to_netcdf( Note that unlimited_dims may also be set via ``dataset.encoding["unlimited_dims"]``. kwargs : - Additonal keyword arguments to be passed to ``xarray.Dataset.to_netcdf`` + Additional keyword arguments to be passed to ``xarray.Dataset.to_netcdf`` """ from .io import _datatree_to_netcdf diff --git a/datatree/tests/test_datatree.py b/datatree/tests/test_datatree.py index 2c775b7c..5854216b 100644 --- a/datatree/tests/test_datatree.py +++ b/datatree/tests/test_datatree.py @@ -109,6 +109,16 @@ def test_setting_node_name_keeps_tree_linkage_and_order(self): # Order was preserved assert list(root.children) == ["one", "second", "three"] + def test_setting_node_to_none(self): + root = DataTree(name="root") + child = DataTree(name="child", parent=root) + + with pytest.raises( + ValueError, + match=r"a node with a parent cannot have its name set to None", + ): + child.name = None + class TestPaths: def test_path_property(self): diff --git a/datatree/treenode.py b/datatree/treenode.py index c430a8b8..edbb408a 100644 --- a/datatree/treenode.py +++ b/datatree/treenode.py @@ -5,6 +5,7 @@ from pathlib import PurePosixPath from typing import ( TYPE_CHECKING, + Any, Generic, Iterator, Mapping, @@ -596,27 +597,21 @@ def name(self) -> str | None: @name.setter def name(self, name: str | None) -> None: - if name is None: - self._name = None - return - - if not isinstance(name, str): - raise TypeError("node name must be a string or None") - if "/" in name: - raise ValueError("node names cannot contain forward slashes") + self._validate_name(name) - parent = self._parent - if parent is None: + if self._parent is None: self._name = name return - old_name = self._name - if old_name is None: - raise ValueError("An unnamed node should not have had a parent") + if name is None: + raise ValueError("a node with a parent cannot have its name set to None") - # Iterate through the ordered dict to preserve the order of children - parent.children = OrderedDict( - ((k if k != old_name else name, v) for k, v in parent.children.items()) + # Rename node while preserving the order of its parent's children + self._parent.children = OrderedDict( + ( + (k if k != self._name else name, v) + for k, v in self._parent.children.items() + ) ) def __str__(self) -> str: @@ -693,3 +688,10 @@ def _path_to_ancestor(self, ancestor: NamedNode) -> NodePath: generation_gap = list(parents_paths).index(ancestor.path) path_upwards = "../" * generation_gap if generation_gap > 0 else "." return NodePath(path_upwards) + + @staticmethod + def _validate_name(name: Any): + if name is not None and not isinstance(name, str): + raise TypeError("node name must be a string or None") + if name is not None and "/" in name: + raise ValueError("node names cannot contain forward slashes") diff --git a/docs/source/whats-new.rst b/docs/source/whats-new.rst index c60d25e9..62f324cb 100644 --- a/docs/source/whats-new.rst +++ b/docs/source/whats-new.rst @@ -46,7 +46,7 @@ Deprecations Bug fixes ~~~~~~~~~ -- Renaming a node also update the key of its parent's children +- Renaming a node also updates the key of its parent's children (:issue:`309`, :pull:`310`) By `Etienne Schalk `_. - Keep attributes on nodes containing no data in :py:func:`map_over_subtree`. (:issue:`278`, :pull:`279`) From 13df1b734320c025781ff950e2865eacd04075e7 Mon Sep 17 00:00:00 2001 From: eschalk Date: Sat, 17 Feb 2024 12:43:49 +0100 Subject: [PATCH 8/8] Updated failing test --- datatree/tests/test_formatting.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/datatree/tests/test_formatting.py b/datatree/tests/test_formatting.py index 0f64644c..76c6e072 100644 --- a/datatree/tests/test_formatting.py +++ b/datatree/tests/test_formatting.py @@ -108,13 +108,13 @@ def test_diff_node_data(self): Data in nodes at position '/a' do not match: Data variables only on the left object: - v int64 1 + v int64 8B 1 Data in nodes at position '/a/b' do not match: Differing data variables: - L w int64 5 - R w int64 6""" + L w int64 8B 5 + R w int64 8B 6""" ) actual = diff_tree_repr(dt_1, dt_2, "equals") assert actual == expected