-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: assorted indexing-related cleanups #31797
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
Changes from 3 commits
fc2fd69
58c6f48
900beba
66ad495
3106a77
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 |
---|---|---|
|
@@ -3190,17 +3190,16 @@ def is_int(v): | |
# convert the slice to an indexer here | ||
|
||
# if we are mixed and have integers | ||
try: | ||
if is_positional and self.is_mixed(): | ||
if is_positional and self.is_mixed(): | ||
try: | ||
# Validate start & stop | ||
if start is not None: | ||
self.get_loc(start) | ||
if stop is not None: | ||
self.get_loc(stop) | ||
is_positional = False | ||
except KeyError: | ||
if self.inferred_type in ["mixed-integer-float", "integer-na"]: | ||
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. this was not needed? 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. not reachable; the check for self.is_mixed() a few lines up means these inferred_types are impossible |
||
raise | ||
except KeyError: | ||
pass | ||
|
||
if is_null_slicer: | ||
indexer = key | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -862,7 +862,9 @@ def __getitem__(self, key): | |
|
||
return result | ||
except InvalidIndexError: | ||
pass | ||
if not isinstance(self.index, MultiIndex): | ||
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. What is this for? 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. it wasn't obvious to me that non-MI shouldn't be allowed through here, this should clarify it for future readers (or me once I forget) 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. Gotcha; does that fix an issue somewhere else? 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. no, just clarity, avoids doing some other checks before raising |
||
raise | ||
|
||
except (KeyError, ValueError): | ||
if isinstance(key, tuple) and isinstance(self.index, MultiIndex): | ||
# kludge | ||
|
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.
iloc
ends up calling the same method, so calling it here directly probably avoids a bunch of duplicate validation.So I would either leave it as is, or at least check the impact of that, or eg change to
iloc._setitem_with_indexer(..)
to make it more obvious that it is positional indexingThere 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.
WIll do that for now and revisit in a non-"cleanup" PR
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.
updated