xarray
xarray copied to clipboard
Forbid path-like DataTree child names #9490
- [x] Closes #9490
- [x] Closes #9978
- [x] Tests added
- [x] User visible changes (including notable bug fixes) are documented in
whats-new.rst
Addresses #9490 and #9978 by forbidding the / character in DataTree child names.
-
DataTreenode names are already checked using_validate_namefrom thetreenodemodule. I therefore call this function inside the_check_childrenmember function ofTreeNode. This check ensures invalidchildrenarguments are caught early in theDataTreeconstructor, and when theDataTree.childrenattribute is reset explicitly (see #9441). I have also reworded the_validate_nameerror message slightly for consistency with_check_for_slashes_in_names(defined in thedatatreemodule) which checks variable names. To illustrate, code like
andimport xarray as xr ds = xr.Dataset({"a": xr.DataArray([1, 2, 3])}) dt = xr.DataTree(dataset=ds, children={"x/y": xr.DataTree()})
will now both produceimport xarray as xr ds = xr.Dataset({"a": xr.DataArray([1, 2, 3])}) dt = xr.DataTree(dataset=ds, children={"x": xr.DataTree()}) dt.children = {"x/y": xr.DataTree()}ValueError: Node name 'x/y' contains the '/' character. Nodes cannot have names containing '/' characters, as this would make path-like access to nodes ambiguous. - I have also added a call to
_check_childrenon the inputchildrenmapping given to theDataTreeconstructor, before it is assigned and the setter function is called. This additional check ensures that if the user passes a badchildren, a helpful error message is given before the shallow copy{name: child.copy() for name, child in children.items()}fails. - I have added a couple of lines to the
DataTree.__init__docstring to communicate that/characters are forbidden in both thenameargument, and the keys of thechildrenmapping argument.
Additional Notes
This PR follows after discussion with @TomNicholas in #11001. See also #9441 and #9477.
I've just realized this PR is very similar to @TomNicholas's PR #9492 from 2024, sorry! Happy to update authorship in the whats-new.rst if appropriate. Not sure why #9492 wasn't merged as a temporary fix for #9485.