Repository navigation
Support path-like keys in DataTree.__contains__ - #11701
Yagnik-Trivedi wants to merge 5 commits into
Conversation
`key in tree` only looked at the current node, while `tree[key]` resolves unix-like paths, so e.g. `"a/b" in tree` was False even though `tree["a/b"]` worked. Fall back to the same path lookup as indexing for string keys that are paths, keeping the fast local check for plain names. Also make `_get_item` raise KeyError instead of AttributeError when a path continues past a variable (e.g. `tree["group/var/x"]`), so that `in` can rely on it. Closes pydata#9354 Co-authored-by: Claude <noreply@anthropic.com>
|
Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient. |
github-actions
Bot
added
the
topic-DataTree
Co-authored-by: Claude <noreply@anthropic.com>
headtr1ck
left a comment
headtr1ck
left a comment
There was a problem hiding this comment.
One question, otherwise this looks good, thanks!
| if isinstance(key, str) and ("/" in key or key in ("", ".", "..")): | ||
| try: | ||
| self._get_item(key) | ||
| except KeyError: |
There was a problem hiding this comment.
I think "//a" would raise a ValueError here, maybe we should catch these errors as well?
Not 100% sure though.
There was a problem hiding this comment.
You’re right: with exactly two leading slashes, POSIX path rules keep // as a separate root, and NodePath rejected it with a ValueError, which leaked out of in. Since "///a" and "a//b" are already normalized, I made NodePath treat a leading // like / as well (8e6b90f). So now "//a" in tree is True and tree["//a"] returns the same node as tree["/a"]. Tests added for both.
…ins__ Paths starting with exactly two slashes (e.g. "//a") are rejected by NodePath with a ValueError, which leaked out of `in`. Co-authored-by: Claude <noreply@anthropic.com>
POSIX keeps exactly two leading slashes as a distinct root, so NodePath rejected paths like "//a" with a ValueError, while "/a" and "///a" both worked. Normalize it in NodePath, so indexing, `in`, from_dict and assignment all accept it, and drop the ValueError catch in __contains__ that is no longer needed. Co-authored-by: Claude <noreply@anthropic.com>
This branch has not been deployed
2 participants
Description
key in treeonly looked at the current node, whiletree[key]resolves unix-like paths. Sotree["a/b"]worked but"a/b" in treereturnedFalse(same for"/a/b","./a","a/b/var",".","/",".."from a child, ...).This PR makes
__contains__follow the same rule as indexing:key in treeisTrueexactly whentree[key]succeeds.self.variables/self.children, which already include inherited coordinates)./, or are"",".","..") go through the same_get_itemlookup that__getitem__uses, so the two can't drift apart again.While doing this I found that
_get_itemraisedAttributeErrorinstead ofKeyErrorwhen a path continues past a variable, e.g.tree["group/var/x"]ortree["group/var/.."], because it kept walking into theDataArray. It now raisesKeyError, which__contains__relies on.Performance: routing every lookup through
_get_itemmade"var" in tree~30x slower (it builds aDatasetView), so plain names short-circuit first. Measured on a node with 50 variables and 50 children: ~1.5 µs before and after for hits and misses.Open questions for reviewers
DataTree.get("a/b")is still local-only, as its docstring says, soinandgetnow differ for path keys. Shouldgetalso accept paths (separate PR)?"a/b" in tree.keys()is nowTruealthough iteratingkeys()only yields local names.Datasetbehaves similarly for coordinates ("x" in dsbut not inlist(ds)), so I left it.Checklist
TestContains: relative/absolute/../self paths, variables at a path, inherited coords, paths past a variable, non-string and unhashable keys; plus a__getitem__regression test)whats-new.rst(also added an example to the hierarchical-data user guide)AI Disclosure
Tools: Claude Code (Claude Opus 5.5). I asked it to find an open issue, reproduce it, and implement a fix covering the edge cases. I chose the issue and the approach (reuse
_get_item;"",".","/"count as present becausetree[key]returns the tree). The code, tests, and this description were written by the agent at my direction.