Make illegal path-like variable names when constructing a DataTree from a Dataset #9378
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @TomNicholas |
TomNicholas
added
the
topic-DataTree
TomNicholas
reviewed
Aug 19, 2024
TomNicholas
reviewed
Aug 21, 2024
xarray/core/datatree.py
Outdated
Comment on lines
166
to
171
| 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. " | ||
| ) |
There was a problem hiding this comment.
I think this is good, but @keewis you mentioned you had a comment didn't you?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure if this has to be considered here, but NetCDF4 and HDF5 do not allow forward slashes in variable/dataset names.
There was a problem hiding this comment.
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.
TomNicholas
reviewed
Sep 9, 2024
TomNicholas
reviewed
Sep 9, 2024
TomNicholas
reviewed
Sep 9, 2024
…s-when-dt-from-ds
Co-authored-by: Tom Nicholas <tom@cworthy.org>
Co-authored-by: Tom Nicholas <tom@cworthy.org>
Co-authored-by: Tom Nicholas <tom@cworthy.org>
|
Integrated the suggested changes and updated the test message, ready to be merged a priori @TomNicholas |
This was referenced Sep 12, 2024
…s-when-dt-from-ds
TomNicholas
approved these changes
Sep 12, 2024
TomNicholas
reviewed
Sep 12, 2024
TomNicholas
reviewed
Sep 12, 2024
shoyer
approved these changes
Sep 12, 2024
4 tasks
dcherian
added a commit
to dcherian/xarray
that referenced
this pull request
Sep 17, 2024
* 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) ...
dcherian
added a commit
to dcherian/xarray
that referenced
this pull request
Sep 17, 2024
* 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)
hollymandel
pushed a commit
to hollymandel/xarray
that referenced
this pull request
Sep 23, 2024
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes Forbid path-like variable names in datatree #9339
Tests added
User visible changes (including notable bug fixes) are documented in
whats-new.rstNew functions/methods are listed inapi.rstOriginal 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
HashablevsstrDataset keysNote: DataTree keys are
Hashable. I only check for slashes in the variable names if they are instance ofstr.I never encountered a case (yet) where a Dataset keys are not
strbutHashablein the broader case.We can imagine corner-cases where keys would be other types of
Hashable, egPathfrompathlibThe 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
Hashablespace ontostrspace: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.