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.
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
Add set_xindex and drop_indexes methods #6971
Add set_xindex and drop_indexes methods #6971
Changes from 19 commits
3f6f637
bf30d54
9de9c46
aa403a4
210a59a
d8c3985
c4afabf
fe723ce
a48c853
01de6bd
201bd05
41c896f
1ec5ca6
a6caa7a
bb07d5a
ec2f8fc
f9601b9
1a555bc
0b7d582
3ab0bc9
9e75f95
00c2711
cb67612
af67168
2cd0aa8
61d6e28
ec08d73
20dbf5a
b598447
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
mypy is not happy with this:
#6971 (comment)
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.
strings are sequences apparently:
Try out
CoordNames = Union[str, Iterable[Hashable]]
seems to be succesful in #7048.It would be nice if we aligned these tricky types so try to use named variables for repeated arguments.
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.
Technically a str is also an Iterable of Hashable :P
But the typing community is quite relaxed about violating that fact.
So as long as you don't need the two types to be "perpendicular" it should work.
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.
+1 for using a named variable like
CoordNames
. The tricky thing here is that the order is important. Do we useSequence
in Xarray in that case? I guess we would need to define two variables for each case where the order does / doesn't matter?Also, I don't remember whether a single coordinate name should be
str
orHashable
. Should we treat it like a single dimension name or not?I feel like this issue should be addressed more globally in Xarray than within the scope of this PR. Perhaps better to move on and merge this PR before the next release?
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.
Usually we try to move to
str | Iterable [Hashable]
for "one or more dims", andHashable
for a single dim.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.
Probably not in all cases? For example, with
DataArray.__init__(..., dims: str | Iterable[Hashable])
the type checker would allow passing a set. Recently I had to figure out what was going on withxr.DataArray(data=np.zeros((10, 5)), dims={'x', 'time'})
, which mypy should actually catch withSequence[Hashable]
. Slightly off-topic: should we have two variablesDims
andOrderedDims
defined inxarray.core.types
?Same issue here for coordinate names.
str | Sequence[Hashable]
seems to work well, though.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.
Why should a set not be allowed?
It's already since quite some time that the order is preserved? I think all built-in Iterables have conserved order, and internally we convert to tuple anyway?
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 don't think the order is preserved for sets (unlike dicts). This is what I can get with CPython 3.9 / Xarray v2022.6.0:
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.
Whoops you are right, that was dicts.
Then indeed we need to distinguish between dims and ordered dims.
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.
Does this mean that you cannot use coords in more than one indexes? (I am not sure how important this is but could imagine a use case where lat & lon are used as 1D indexes and in a KDTree).
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.
Yes that's right, allow multiple indexes per coordinate would make many things much harder.
There are indeed some examples (like the one you mention) where it could be useful to have multiple indexes. But I think it could be solved by either switching between indexes (if building them is not too expensive) or via a custom "meta-index" that would encapsulate both kinds of indexes.
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.
Fair enough - thanks for the clarification!