-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API: consistency with .ix and .loc for getitem operations (GH8613) #9566
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
Conversation
f5b9f7e
to
d61b9b5
Compare
cc @immerrr I think this should resolve things and make |
|
||
The behavior of a small sub-set of edge cases for indexers have changed (:issue:`8613`): | ||
|
||
- to conform the intent of ``.loc`` to allow the same behavior for slice indexing as ``.ix`` when indexers are not found |
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 would here also explain it in words, something like "slicing where the start and/or stop bound is not found in the index is now allowed (previously gave KeyError), as is the case for ix
. The change is only for slicing, not when indecxing with a single label."
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.
done
1141055
to
2c2cf61
Compare
@@ -1243,25 +1243,7 @@ def _has_valid_type(self, key, axis): | |||
# boolean | |||
|
|||
if isinstance(key, slice): | |||
|
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.
👍 to deleting lines from core.indexing
.
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.
whoo hoo!
First of all, big thanks for working on this, @jreback! However, I don't think we need to enforce distinctions between float and int for slice bounds, because there is no ambiguity in the ordering between integers and floats (whereas the ordering with respect to strings, for example, is indeed problematic). Users shouldn't have to check the type of their index (if they know it's numeric) before doing indexing. In particular:
The binary search logic for determining slice bound locations should already work for both of these cases. |
To give a more concrete example from my experience: an index corresponding to a variable like |
I added in these checks, mainly because numpy is deprecating float indexers (IIRC it has a warning now), and we have been warning on it. Its easy to take out, but I think that indexing with a float on an integer index is really not in the though it IS convenient. currently we don't allow this for simple example from 0.15.2
These all fail with |
@jorisvandenbossche what do you think? |
I agree that using float indexers is a based idea for position based indexing ( But indexing with |
this should then also work for |
To elaborate: There is never a good reason to use floats for position based indexing. Numpy warns and/or raises because using a float is an indication that your code probably has a bug. In contrast, there certainly are good reasons to use floats for label based indexing, because the labels might include floats themselves (or might not, depending on dtypes). |
If I had my way, we would make some hard choices and deprecate/remove fall-back indexing all together (#9213). In the mean-time, I suspect the best approach is to simply try to break/change as little existing code as possible. |
ok, here is the change (plus making some testing more robust)
I could enable for |
I agree with the logic of allowing float indexers for integer indexes when using Ideally, I think it should also work for For |
ok, I added the .ix support for float indexers on an Int index. It is trivial to make this work for |
But for |
Example:
|
ok, is updated, any more comments |
Notes | ||
----- | ||
Value of `side` parameter should be validated in caller. | ||
|
||
""" | ||
|
||
# pass thru float indexers if we have a numeric type index |
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 logic here seems OK, but a little confusing in context. To me it would make more sense to put it on the subclass _maybe_cast_slice_bound
methods, even if that means a little more duplication. Otherwise, things will break, e.g., as soon as we add IntervalIndex
.
Generally seems fine by me. |
raise TypeError rather than KeyError on invalid scalar/slice indexing with that index type
indexing is such a rabbit hole :) I cleaned up its tricky because you have the kinds of indexers (ix/loc/iloc/getitem) x index_types (Index,Float,Int,Datetime....) x kinds of indexing (slice/scalar) So its better (meaning slightly cleaner), with the semantics we discussed above. |
rename typ -> kind for _convert_*_indexer add kind argument to _maybe_cast_slice_bound cleaned up _convert_slice_indexer a bit
API: consistency with .ix and .loc for getitem operations (GH8613)
Thanks a lot for this! |
Indeed, greatly appreciated! |
That's great that you guys (and especially @jreback) actually went through this 👍 |
trying to make indexing slightly less of a rabbit hole :) |
closes #8613
Slicing with float indexers; now works for ix (loc worked before)