Make illegal path-like variable names when constructing a DataTree from a Dataset by etienneschalk · Pull Request #9378 · pydata/xarray

etienneschalk · 2024-08-18T12:11:11Z

Closes Forbid path-like variable names in datatree #9339
Tests added
User visible changes (including notable bug fixes) are documented in whats-new.rst
~~New functions/methods are listed in api.rst~~
Original issue on the DataTree repository : When creating a DataTree from a Dataset with path-like variable, subgroups are expected to be created xarray-contrib/datatree#311
Original PR on the DataTree repository : Make illegal path-like variable names when constructing a DataTree from a Dataset xarray-contrib/datatree#314

Technical Note

Regarding `Hashable` vs `str` Dataset keys

Note: DataTree keys are Hashable. I only check for slashes in the variable names if they are instance of str.
I never encountered a case (yet) where a Dataset keys are not str but Hashable in the broader case.
We can imagine corner-cases where keys would be other types of Hashable, eg Path from pathlib

In [2]: frompathlibimportPathIn [3]: hash(Path("/"))
Out[3]: -3809984204556177651

The choice I made is (1): only apply the check of slashes in the key if the key is an instance of str.
Another choice (2)would be to project the Hashable space onto str space: str(variable_name)
(1) seems more conservative than (2) as I do not pretend to be able to get a string representation for any Hashable.

…om a Dataset

etienneschalk · 2024-08-18T13:27:38Z

cc @TomNicholas

TomNicholas

Thanks @etienneschalk ! Just two small comments.

TomNicholas · 2024-08-21T19:04:59Z

+ raise KeyError(
+ "Given Dataset contains variable names with '/': "
+ f"{offending_variable_names}. "
+ "A Dataset represents a group, and a single group "
+ "cannot have path-like variable names with '/' characters in them. "
+ )


I think this is good, but @keewis you mentioned you had a comment didn't you?

I think the improvement to be made here is to clarify that currently slashes are allowed in variable names on xr.Dataset, but are forbidden in variable names in xr.DataTree.

I'm not sure if this has to be considered here, but NetCDF4 and HDF5 do not allow forward slashes in variable/dataset names.

Interesting. I think that doesn't change anything for this PR, so long as that restriction is already explicitly accounted for in the respective backend save methods.

…s-when-dt-from-ds

Co-authored-by: Tom Nicholas <tom@cworthy.org>

etienneschalk · 2024-09-10T20:30:39Z

Integrated the suggested changes and updated the test message, ready to be merged a priori @TomNicholas

…s-when-dt-from-ds

shoyer

LGTM, thanks!

It would also be nice to add a check like this for child names.

* main: (26 commits) Forbid modifying names of DataTree objects with parents (pydata#9494) DAS-2155 - Merge datatree documentation into main docs. (pydata#9033) Make illegal path-like variable names when constructing a DataTree from a Dataset (pydata#9378) Ensure TreeNode doesn't copy in-place (pydata#9482) `open_groups` for zarr backends (pydata#9469) Update pyproject.toml (pydata#9484) New whatsnew section (pydata#9483) Release notes for v2024.09.0 (pydata#9480) Fix `DataTree.coords.__setitem__` by adding `DataTreeCoordinates` class (pydata#9451) Rename DataTree's "ds" and "data" to "dataset" (pydata#9476) Update DataTree repr to indicate inheritance (pydata#9470) Bump pypa/gh-action-pypi-publish in the actions group (pydata#9460) Repo checker (pydata#9450) Add days_in_year and decimal_year to dt accessor (pydata#9105) remove parent argument from DataTree.__init__ (pydata#9465) Fix inheritance in DataTree.copy() (pydata#9457) Implement `DataTree.__delitem__` (pydata#9453) Add ASV for datatree.from_dict (pydata#9459) Make the first argument in DataTree.from_dict positional only (pydata#9446) Fix typos across the code, doc and comments (pydata#9443) ...

* main: Opt out of floor division for float dtype time encoding (pydata#9497) fixed formatting for whats-new (pydata#9493) Forbid modifying names of DataTree objects with parents (pydata#9494) DAS-2155 - Merge datatree documentation into main docs. (pydata#9033) Make illegal path-like variable names when constructing a DataTree from a Dataset (pydata#9378) Ensure TreeNode doesn't copy in-place (pydata#9482) `open_groups` for zarr backends (pydata#9469) Update pyproject.toml (pydata#9484) New whatsnew section (pydata#9483)

…om a Dataset (pydata#9378) * Make illegal path-like variable names when constructing a DataTree from a Dataset * Updated whats-new.rst * PR comments * Revert diff * Update xarray/core/datatree.py Co-authored-by: Tom Nicholas <tom@cworthy.org> * Update xarray/core/datatree.py Co-authored-by: Tom Nicholas <tom@cworthy.org> * Update xarray/tests/test_datatree.py Co-authored-by: Tom Nicholas <tom@cworthy.org> * Update expected Exception message in test * Merge changes from pydata#9476 * Fix --------- Co-authored-by: Tom Nicholas <tom@cworthy.org>

Make illegal path-like variable names when constructing a DataTree fr…
a3972f5
…om a Dataset

etienneschalk marked this pull request as draft August 18, 2024 12:11

Updated whats-new.rst
c8ce30a

etienneschalk marked this pull request as ready for review August 18, 2024 12:49

TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label Aug 19, 2024

TomNicholas reviewed Aug 19, 2024
View reviewed changes

Comment threadxarray/core/datatree.py Outdated
Comment threadxarray/core/datatree.py Outdated

etienneschalk added 2 commits August 19, 2024 22:03

PR comments
6bf0982

Revert diff
fd3edff

TomNicholas reviewed Aug 21, 2024
View reviewed changes

TomNicholas reviewed Sep 9, 2024
View reviewed changes

Comment threadxarray/core/datatree.py Outdated

TomNicholas added the plan to merge Final call for comments label Sep 9, 2024

TomNicholas reviewed Sep 9, 2024
View reviewed changes

Comment threadxarray/core/datatree.py Outdated

TomNicholas reviewed Sep 9, 2024
View reviewed changes

Comment threadxarray/tests/test_datatree.py Outdated

etienneschalkand others added 5 commits September 10, 2024 21:57

Merge branch 'main' into eschalk/make-illegal-path-like-variable-name…
19837b7
…s-when-dt-from-ds

Update xarray/core/datatree.py
bc15c1c
Co-authored-by: Tom Nicholas <tom@cworthy.org>

Update xarray/core/datatree.py
f7af02d
Co-authored-by: Tom Nicholas <tom@cworthy.org>

Update xarray/tests/test_datatree.py
7d2289f
Co-authored-by: Tom Nicholas <tom@cworthy.org>

Update expected Exception message in test
107603b

This was referenced Sep 12, 2024
Assigning to DataTree.coords should support full paths #9485
Open
Track merging datatree into xarray #8572
Closed

Merge branch 'main' into eschalk/make-illegal-path-like-variable-name…
e7ab84d
…s-when-dt-from-ds

TomNicholas approved these changes Sep 12, 2024
View reviewed changes

TomNicholas reviewed Sep 12, 2024
View reviewed changes

Comment threadxarray/core/datatree.py Outdated

Merge changes frompydata#9476
95384ac

TomNicholas reviewed Sep 12, 2024
View reviewed changes

Comment threadxarray/core/datatree.py Outdated

Fix
5886af5

shoyer approved these changes Sep 12, 2024
View reviewed changes

TomNicholas mentioned this pull request Sep 12, 2024
Forbid path-like child names #9490
Open

TomNicholas merged commit 8bd456c into pydata:mainSep 12, 2024

TomNicholas mentioned this pull request Sep 13, 2024
Forbid slashes in datatree coordinate names #9492
Open
4 tasks

kmuehlbauer mentioned this pull request Sep 18, 2024
Forbid path-like variable names in datatree #9339
Closed

eshort0401 mentioned this pull request Dec 11, 2025
Fix DataTree path like assignment GH9485 GH9490 GH9978 #11001
Closed
3 tasks

Navigation Menu

Uh oh!

Uh oh!

Make illegal path-like variable names when constructing a DataTree from a Dataset - #9378

Make illegal path-like variable names when constructing a DataTree from a Dataset#9378
TomNicholas merged 12 commits into
pydata:mainfrom
etienneschalk:eschalk/make-illegal-path-like-variable-names-when-dt-from-ds

etienneschalk commented Aug 18, 2024•
edited
Loading

Uh oh!

etienneschalk commented Aug 18, 2024

Uh oh!

TomNicholas left a comment

Uh oh!

Uh oh!

Uh oh!

TomNicholasAug 21, 2024

Uh oh!

TomNicholasAug 27, 2024

Uh oh!

kmuehlbauerAug 27, 2024

Uh oh!

TomNicholasAug 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienneschalk commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

shoyer left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

etienneschalk commented Aug 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Technical Note

Regarding Hashable vs str Dataset keys

Uh oh!

etienneschalk commented Aug 18, 2024

Uh oh!

TomNicholas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TomNicholasAug 21, 2024

Choose a reason for hiding this comment

Uh oh!

TomNicholasAug 27, 2024

Choose a reason for hiding this comment

Uh oh!

kmuehlbauerAug 27, 2024

Choose a reason for hiding this comment

Uh oh!

TomNicholasAug 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienneschalk commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

shoyer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

etienneschalk commented Aug 18, 2024•
edited
Loading

Regarding `Hashable` vs `str` Dataset keys