Skip to content

Better error messages for DataTree #9222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion xarray/core/datatree.py
Original file line number Diff line number Diff line change
@@ -198,7 +198,7 @@ def __init__(
coords: Mapping[Any, Any] | None = None,
attrs: Mapping[Any, Any] | None = None,
):
raise AttributeError("DatasetView objects are not to be initialized directly")
raise TypeError("DatasetView objects are not to be initialized directly")

@classmethod
def _constructor(
16 changes: 9 additions & 7 deletions xarray/core/treenode.py
Original file line number Diff line number Diff line change
@@ -526,14 +526,14 @@ def _get_item(self: Tree, path: str | NodePath) -> Tree | T_DataArray:
for part in parts:
if part == "..":
if current_node.parent is None:
raise KeyError(f"Could not find node at {path}")
raise KeyError(path)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you've made this error message less informative @shoyer ? The equivalent operation on a Dataset gives a more explicit message:

xr.Dataset()['a']
KeyError: "No variable named 'a'. Variables on the dataset include []"

else:
current_node = current_node.parent
elif part in ("", "."):
pass
else:
if current_node.get(part) is None:
raise KeyError(f"Could not find node at {path}")
raise KeyError(path)
else:
current_node = current_node.get(part)
return current_node
@@ -581,7 +581,7 @@ def _set_item(
path = NodePath(path)

if not path.name:
raise ValueError("Can't set an item under a path which has no name")
raise ValueError("cannot set an item under a path which has no name")

if path.root:
# absolute path
@@ -598,7 +598,7 @@ def _set_item(
if part == "..":
if current_node.parent is None:
# We can't create a parent if `new_nodes_along_path=True` as we wouldn't know what to name it
raise KeyError(f"Could not reach node at path {path}")
raise ValueError(f"could not reach node at path {path}")
else:
current_node = current_node.parent
elif part in ("", "."):
@@ -612,14 +612,14 @@ def _set_item(
current_node._set(part, new_node)
current_node = current_node.children[part]
else:
raise KeyError(f"Could not reach node at path {path}")
raise ValueError(f"could not reach node at path {path}")

if name in current_node.children:
# Deal with anything already existing at this location
if allow_overwrite:
current_node._set(name, item)
else:
raise KeyError(f"Already a node object at path {path}")
raise ValueError(f"already a node object at path {path}")
else:
current_node._set(name, item)

@@ -630,7 +630,9 @@ def __delitem__(self: Tree, key: str) -> None:
del self._children[key]
child.orphan()
else:
raise KeyError(key)
raise KeyError(key) from ValueError(
"cannot delete key not found in child nodes"
)

def same_tree(self, other: Tree) -> bool:
"""True if other node is in the same tree as this node."""
6 changes: 3 additions & 3 deletions xarray/tests/test_treenode.py
Original file line number Diff line number Diff line change
@@ -204,7 +204,7 @@ def test_child_already_exists(self):
mary: TreeNode = TreeNode()
john: TreeNode = TreeNode(children={"Mary": mary})
mary_2: TreeNode = TreeNode()
with pytest.raises(KeyError):
with pytest.raises(ValueError):
john._set_item("Mary", mary_2, allow_overwrite=False)

def test_set_grandchild(self):
@@ -225,7 +225,7 @@ def test_create_intermediate_child(self):
rose: TreeNode = TreeNode()

# test intermediate children not allowed
with pytest.raises(KeyError, match="Could not reach"):
with pytest.raises(ValueError, match="could not reach"):
john._set_item(path="Mary/Rose", item=rose, new_nodes_along_path=False)

# test intermediate children allowed
@@ -244,7 +244,7 @@ def test_overwrite_child(self):

# test overwriting not allowed
marys_evil_twin: TreeNode = TreeNode()
with pytest.raises(KeyError, match="Already a node object"):
with pytest.raises(ValueError, match="already a node object"):
john._set_item("Mary", marys_evil_twin, allow_overwrite=False)
assert john.children["Mary"] is mary
assert marys_evil_twin.parent is None