-
-
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
More flexible index variables #8124
Draft
benbovy
wants to merge
10
commits into
pydata:main
Choose a base branch
from
benbovy:non-pandas-index-variables
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
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
Used to wrap indexed coordinate data. It raises an explicit error message when trying to modify the array values in-place.
Used to create the variables from the index object and then wrap coordinate data to prevent updating values in-place.
Except for PandasIndex objects: not needed for now since they create IndexVariable objects.
2 tasks
4 tasks
5 tasks
5 tasks
4 tasks
4 tasks
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.
whats-new.rst
api.rst
The goal of this PR is to provide a more general solution to indexed coordinate variables, i.e., support arbitrary dimensions and/or duck arrays for those variables while at the same time prevent them from being updated in a way that would invalidate their index.
This would solve problems like the one mentioned here: #1650 (comment)
@shoyer I've tried to implement what you have suggested in #4979 (comment). It would be nice indeed if eventually we could get rid of
IndexVariable
. It won't be easy to deprecate it until we finish the index refactor (i.e., all methods listed in #6293), though. Also, I didn't find an easy way to refactor that class as it has been designed too closely around a 1-d variable backed by apandas.Index
.So the approach implemented in this PR is to keep using
IndexVariable
for PandasIndex until we can deprecate / remove it later, and for the other cases useVariable
with data wrapped in a customIndexedCoordinateArray
object.The latter solution (wrapper) doesn't always work nicely, though. For example, several methods of
Variable
expect thatself._data
directly returns a duck array (e.g., a dask array or a chunked duck array). A wrapped duck array will result in unexpected behavior there. We could probably add some checks / indirection or extend the wrapper API... But I wonder if there wouldn't be a more elegant approach?More generally, which operations should we allow / forbid / skip for an indexed coordinate variable?
(Note: we could add
Index.chunk()
andIndex.load()
methods in order to allow an Xarray index implement custom logic for the two latter cases like, e.g., convert a DaskIndex to a PandasIndex during load, see #8128).cc @andersy005 (some changes made here may conflict with what you are refactoring in #8075).