Repository: pydata/xarray

Issue:
Consistency of names of stored children / dataarrays with keys in wrapping object
### What is your issue?

In https://github.com/xarray-contrib/datatree/issues/309 @marcel-goldschen-ohm pointed out some counterintuitive behaviour of the `DataTree.name` setter.

```python
# using xarray main
In [2]: from xarray.core.datatree import DataTree

In [3]: root = DataTree.from_dict(
   ...:     {
   ...:         'child': DataTree(),
   ...:     },
   ...:     name='root',
   ...: )

In [4]: root['child'].name = 'childish'

In [5]: root
Out[5]: 
<xarray.DataTree 'root'>
Group: /
└── Group: /childish
```

this looks fine, but

```python
In [6]: root.children
Out[6]: 
Frozen({'child': <xarray.DataTree 'childish'>
Group: /childish})
```
which means that
```python
In [14]: root['childish']
```
```python
KeyError: 'Could not find node at childish'
```

Okay, so we just fix it so that setting `.name` changes the key in the parent node right? But as @etienneschalk pointed out in https://github.com/xarray-contrib/datatree/issues/309#issuecomment-1949940406, that wouldn't be consistent with `DataArray` and `Dataset` names behave: they don't update the key in the wrapping object either. Instead they just completely ignore that you asked to update it at all:

```python
In [15]: ds = xr.Dataset({'a': 0})

In [17]: ds['a'].name = 'b'

In [18]: ds
Out[18]: 
<xarray.Dataset> Size: 8B
Dimensions:  ()
Data variables:
    a        int64 8B 0

In [19]: ds.variables
Out[19]: 
Frozen({'a': <xarray.Variable ()> Size: 8B
array(0)})
```

This happens because `Dataset` objects do not actually store `DataArray` objects, they only store `Variables` (which are un-named), and reconstruct a new `DataArray` upon `__getitem__` calls. So if you use `.name` to alter the name of that newly-constructed `DataArray` you're not actually touching the wrapping `Dataset` at all, and therefore have no way to update its keys.

Indeed the same thing happens (for the same reason) when altering the name of a Variable that is part of a `DataTree` node:

```python
In [8]: import xarray as xr

In [9]: root['a'] = xr.DataArray(0)

In [10]: root
Out[10]: 
<xarray.DataTree 'root'>
Group: /
│   Dimensions:  ()
│   Data variables:
│       a        int64 8B 0
└── Group: /child

In [11]: root['a'].name
Out[11]: 'a'

In [12]: root['a'].name = 'b'

In [13]: root['a']
Out[13]: 
<xarray.DataArray 'a' ()> Size: 8B
array(0)
```

Is this ignoring of the name setter intended, or a bug? And should `DataTree` follow the behaviour of `Dataset` here or not? If it doesn't follow the behaviour of `Dataset` then we're going to end up with a situation where

```python
root['child'].name = 'new_name'
```
actually makes a change to `root`, but
```python
root['a'].name = 'new_name'
```
does not, so the behaviour of `.name` depends on whether or not your key corresponds to a `DataTree` value or a `Variable` value. 🫤 

In the `DataTree` case I think it is a lot easier to update the key in the wrapping object when `.name` is set because there is a bi-directional linkage back to the parent node, which doesn't exist to link the `DataArray` instance back to `Dataset`.

cc @shoyer - I would appreciate your thoughts here.

Discussion:

Patch:
diff --git a/xarray/core/treenode.py b/xarray/core/treenode.py
index d74c8217..17accf74 100644
--- a/xarray/core/treenode.py
+++ b/xarray/core/treenode.py
@@ -640,6 +640,14 @@ class TreeNode(Generic[Tree]):
 AnyNamedNode = TypeVar("AnyNamedNode", bound="NamedNode")
 
 
+def _validate_name(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")
+
+
 class NamedNode(TreeNode, Generic[Tree]):
     """
     A TreeNode which knows its own name.
@@ -653,8 +661,8 @@ class NamedNode(TreeNode, Generic[Tree]):
 
     def __init__(self, name=None, children=None):
         super().__init__(children=children)
-        self._name = None
-        self.name = name
+        _validate_name(name)
+        self._name = name
 
     @property
     def name(self) -> str | None:
@@ -663,11 +671,13 @@ class NamedNode(TreeNode, Generic[Tree]):
 
     @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")
+        if self.parent is not None:
+            raise ValueError(
+                "cannot set the name of a node which already has a parent. "
+                "Consider creating a detached copy of this node via .copy() "
+                "on the parent node."
+            )
+        _validate_name(name)
         self._name = name
 
     def __repr__(self, level=0):
@@ -677,11 +687,13 @@ class NamedNode(TreeNode, Generic[Tree]):
         return repr_value
 
     def __str__(self) -> str:
-        return f"NamedNode('{self.name}')" if self.name else "NamedNode()"
+        name_repr = repr(self.name) if self.name is not None else ""
+        return f"NamedNode({name_repr})"
 
     def _post_attach(self: AnyNamedNode, parent: AnyNamedNode, name: str) -> None:
         """Ensures child has name attribute corresponding to key under which it has been stored."""
-        self.name = name
+        _validate_name(name)  # is this check redundant?
+        self._name = name
 
     def _copy_node(
         self: AnyNamedNode,


Test patch:
diff --git a/xarray/tests/test_datatree.py b/xarray/tests/test_datatree.py
index 0435b199..22c2cae7 100644
--- a/xarray/tests/test_datatree.py
+++ b/xarray/tests/test_datatree.py
@@ -24,10 +24,28 @@ class TestTreeCreation:
         assert dt.children == {}
         assert_identical(dt.to_dataset(), xr.Dataset())
 
-    def test_unnamed(self):
+    def test_name(self):
         dt = DataTree()
         assert dt.name is None
 
+        dt = DataTree(name="foo")
+        assert dt.name == "foo"
+
+        dt.name = "bar"
+        assert dt.name == "bar"
+
+        dt = DataTree(children={"foo": DataTree()})
+        assert dt["/foo"].name == "foo"
+        with pytest.raises(
+            ValueError, match="cannot set the name of a node which already has a parent"
+        ):
+            dt["/foo"].name = "bar"
+
+        detached = dt["/foo"].copy()
+        assert detached.name == "foo"
+        detached.name = "bar"
+        assert detached.name == "bar"
+
     def test_bad_names(self):
         with pytest.raises(TypeError):
             DataTree(name=5)  # type: ignore[arg-type]