-
-
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
Better default behavior of the Coordinates constructor #8107
Better default behavior of the Coordinates constructor #8107
Conversation
... for any input dimension coordinate, if ``indexes=None``. Also, if another ``Coordinates`` object is passed, extract its indexes and raise if ``indexes`` is not None (no align/merge supported here).
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.
These changes look great to me, thank you!
I think it is OK getting them in even if they break a few edges cases.
I also think this is totally fine. |
variables = {k: as_variable(v) for k, v in coords.items()} | ||
variables = {} | ||
for name, data in coords.items(): | ||
var = as_variable(data, name=name) |
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 still converts any coordinate input in the form {"x": array_like}
to an IndexVariable
, which triggers data loading by coercing array_like
to a pd.Index
.
It will be fixed in another PR (#8124).
As a workaround, {"x": ("x", array_like)}
does not coerce array_like
.
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.
@benbovy I'm pretty sure this workaround currently does not work. I tried to create a Coordinates
object from scratch with no indexes and the only way I was able to do it required reverting xarray to v2023.08.0
, i.e. just before this PR was merged.
See the end of https://gist.github.com/TomNicholas/d9eb8ac81d3fd214a23b5e921dbd72b7 for full context.
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.
Raised with MCVE in #8704
* ``Coordinates.__init__`` create default indexes ... for any input dimension coordinate, if ``indexes=None``. Also, if another ``Coordinates`` object is passed, extract its indexes and raise if ``indexes`` is not None (no align/merge supported here). * add docstring examples * fix doctests * fix tests * update what's new
* as_variable: deprecate converting to IndexVariable * fix multi-index edge case * Better default behavior of the Coordinates constructor (#8107) * ``Coordinates.__init__`` create default indexes ... for any input dimension coordinate, if ``indexes=None``. Also, if another ``Coordinates`` object is passed, extract its indexes and raise if ``indexes`` is not None (no align/merge supported here). * add docstring examples * fix doctests * fix tests * update what's new * fix deprecation warning after unintentionally reverted a valid previous change. * avoid unnecessary auto-creation of index to avoid userwarning * catch expected FutureWarnings in test_as_variable * check for coercion to IndexVariable * whatsnew --------- Co-authored-by: Benoit Bovy <benbovy@gmail.com>
whats-new.rst
After working more on
Coordinates
I realize that the default behavior of its constructor could be more consistent with other Xarray objects. This PR changes this default behavior such that:indexes=None
(default). To create dimension coordinates with no index, just passindexes={}
.Coordinates
object is passed as input, its indexes are also added to the new created object. Since we don't support alignment / merge here, the following call raises an error:xr.Coordinates(coords=xr.Coordinates(...), indexes={...})
.This PR introduces a breaking change since
Coordinates
are now exposed in v2023.8.0, which has just been released. It is a bit unfortunate but I think it may be OK for a fresh feature, especially if the next release will be soon after this one.