-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: handle non-iterable hashables as dim names #10638
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -47,6 +47,7 @@ | |||||
is_duck_dask_array, | ||||||
maybe_coerce_to_str, | ||||||
) | ||||||
from xarray.namedarray._typing import _DimsLike | ||||||
from xarray.namedarray.core import NamedArray, _raise_if_any_duplicate_dimensions | ||||||
from xarray.namedarray.parallelcompat import get_chunked_array_type | ||||||
from xarray.namedarray.pycompat import ( | ||||||
|
@@ -369,7 +370,7 @@ class Variable(NamedArray, AbstractArray, VariableArithmetic): | |||||
|
||||||
def __init__( | ||||||
self, | ||||||
dims, | ||||||
dims: _DimsLike, | ||||||
data: T_DuckArray | ArrayLike, | ||||||
attrs=None, | ||||||
encoding=None, | ||||||
|
@@ -378,10 +379,14 @@ def __init__( | |||||
""" | ||||||
Parameters | ||||||
---------- | ||||||
dims : str or sequence of str | ||||||
Name(s) of the the data dimension(s). Must be either a string (only | ||||||
for 1D data) or a sequence of strings with length equal to the | ||||||
dims : Hashable or sequence of Hashable | ||||||
Name(s) of the the data dimension(s). Must be either a Hashable | ||||||
(only for 1D data) or a sequence of Hashables with length equal to the | ||||||
number of dimensions. | ||||||
|
||||||
Note: Tuples are treated as sequences, so ('a', 'b') means two | ||||||
dimensions named 'a' and 'b'. To use a tuple as a single dimension | ||||||
name, wrap it in a list: [('a', 'b')]. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
data : array_like | ||||||
Data array which supports numpy-like data access. | ||||||
attrs : dict_like or None, optional | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -500,12 +500,31 @@ def dims(self, value: _DimsLike) -> None: | |
self._dims = self._parse_dimensions(value) | ||
|
||
def _parse_dimensions(self, dims: _DimsLike) -> _Dims: | ||
dims = (dims,) if isinstance(dims, str) else tuple(dims) | ||
original_dims = dims # Keep reference to original input for error messages | ||
|
||
if isinstance(dims, str): | ||
dims = (dims,) | ||
elif isinstance(dims, Iterable): | ||
dims = tuple(dims) | ||
else: | ||
# Single non-string, non-iterable hashable (int, UUID, etc.) | ||
dims = (dims,) | ||
Comment on lines
+505
to
+511
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would be nice to invert the check to promote correct typing. |
||
|
||
if len(dims) != self.ndim: | ||
raise ValueError( | ||
f"dimensions {dims} must have the same length as the " | ||
f"number of data dimensions, ndim={self.ndim}" | ||
) | ||
# Provide a more helpful error message that explains the tuple ambiguity | ||
if isinstance(original_dims, tuple) and len(dims) > 1 and self.ndim == 1: | ||
raise ValueError( | ||
f"You passed dims={original_dims} for 1-dimensional data. " | ||
f"This is ambiguous: did you mean {len(dims)} separate dimensions, " | ||
f"or a single dimension with tuple name {original_dims}? " | ||
f"For a single tuple-named dimension, use dims=[{original_dims}]. " | ||
f"For multiple dimensions, use {len(dims)}-dimensional data." | ||
) | ||
else: | ||
raise ValueError( | ||
f"dimensions {dims} must have the same length as the " | ||
f"number of data dimensions, ndim={self.ndim}" | ||
) | ||
if len(set(dims)) < len(dims): | ||
repeated_dims = {d for d in dims if dims.count(d) > 1} | ||
warnings.warn( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should recommend what xarray tries to use internally first,
tuple[Hashable, ...]
.