-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Opt out of auto creating index variables #8711
Changes from all commits
07b9856
be99673
c07bfbf
a3116f7
0f70805
6bbcc8a
6bb7149
3581a44
5649a18
66a6fd7
55c0b19
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 |
---|---|---|
|
@@ -33,6 +33,7 @@ | |
decode_numpy_dict_values, | ||
drop_dims_from_indexers, | ||
either_dict_or_kwargs, | ||
emit_user_level_warning, | ||
ensure_us_time_resolution, | ||
infix_dims, | ||
is_dict_like, | ||
|
@@ -80,7 +81,9 @@ class MissingDimensionsError(ValueError): | |
# TODO: move this to an xarray.exceptions module? | ||
|
||
|
||
def as_variable(obj: T_DuckArray | Any, name=None) -> Variable | IndexVariable: | ||
def as_variable( | ||
obj: T_DuckArray | Any, name=None, auto_convert: bool = True | ||
) -> Variable | IndexVariable: | ||
"""Convert an object into a Variable. | ||
|
||
Parameters | ||
|
@@ -100,6 +103,9 @@ def as_variable(obj: T_DuckArray | Any, name=None) -> Variable | IndexVariable: | |
along a dimension of this given name. | ||
- Variables with name matching one of their dimensions are converted | ||
into `IndexVariable` objects. | ||
auto_convert : bool, optional | ||
For internal use only! If True, convert a "dimension" variable into | ||
an IndexVariable object (deprecated). | ||
|
||
Returns | ||
------- | ||
|
@@ -150,9 +156,15 @@ def as_variable(obj: T_DuckArray | Any, name=None) -> Variable | IndexVariable: | |
f"explicit list of dimensions: {obj!r}" | ||
) | ||
|
||
if name is not None and name in obj.dims and obj.ndim == 1: | ||
# automatically convert the Variable into an Index | ||
obj = obj.to_index_variable() | ||
if auto_convert: | ||
if name is not None and name in obj.dims and obj.ndim == 1: | ||
# automatically convert the Variable into an Index | ||
emit_user_level_warning( | ||
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. Maybe we should remove this warning since we're not exactly sure yet what the future behavior will be? Or at least let's make sure this warning won't be emitted in user code unless explicitly calling 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. Okay, so I tried to ensure that this warning won't be emitted in user code simply by replacing the warning with an exception and running all the tests to see where it raised. The only places it raised were:
Is that sufficient? 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. Yes that may be enough for now. |
||
f"variable {name!r} with name matching its dimension will not be " | ||
"automatically converted into an `IndexVariable` object in the future.", | ||
FutureWarning, | ||
) | ||
obj = obj.to_index_variable() | ||
|
||
return obj | ||
|
||
|
@@ -706,8 +718,10 @@ def _broadcast_indexes_vectorized(self, key): | |
variable = ( | ||
value | ||
if isinstance(value, Variable) | ||
else as_variable(value, name=dim) | ||
else as_variable(value, name=dim, auto_convert=False) | ||
) | ||
if variable.dims == (dim,): | ||
variable = variable.to_index_variable() | ||
if variable.dtype.kind == "b": # boolean indexing case | ||
(variable,) = variable._nonzero() | ||
|
||
|
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.
This place is (I think) the only place where a newly created "dimension coordinate" might not always be an IndexVariable (i.e., when an empty dictionary or any other value than
None
is passed as theindexes
argument). It would be nice to add a test for this specific case.The other refactored
as_variable
places in this PR always create an IndexVariable for a dimension coordinate so I think it is covered by the existing tests (invariant checks).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.
I've read this comment many times over, and I don't understand why this test isn't already testing the case you're asking about (i.e. passing
indexes={}
to theCoordinates
constructor, hence preventing anIndexVariable
being automatically created).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.
We might want to add a check in that specific test to make sure that the created "x" variable is not coerced to an
IndexVariable
(before this PR it was still the case even though noxr.indexes.PandasIndex
was created from it).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.
Okay I added a check both explicitly and for if that warning is raised.