-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
PERF: Faster Series.__getattribute__ #20834
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 1 commit
ad6a202
55072a0
ad871ce
4b7334c
857364f
e0710f3
1b598d0
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 |
---|---|---|
|
@@ -4375,7 +4375,7 @@ def __getattr__(self, name): | |
name in self._accessors): | ||
return object.__getattribute__(self, name) | ||
else: | ||
if name in self._info_axis: | ||
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 is not the right fix. The issue is that some index types DO allow a non-holding index type to be testing in Instead I would create a Note we already have 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.
These can't be valid identifiers though, so they by definition can't be going through We can perform this cheap check at the class level, without having to deal with values at all. 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. Note that this isn't addressing the other performance problems with DTI, which are left for #17754 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. as I said, this is not a good solution here. Call a method on the object. In the future folks will have no idea this is called, nor where it is, it add unecessary context and complexity. 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.
We have a comment explaining it on the base Index class and a link back to the original issue. In what case will this cause issues? It's short-circuting a check that will always return False. 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. Agreed with Joris. This if block is specifically for dot selection. It makes sense to be there. 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. my point is that you instead should be calling a function like
putting this here buries all kinds of logic in an obscure routine. no-one will be able to understand / find this later. 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. Why would it be a function? To outcome doesn't depend on
Can you say exactly what logic you think I'm burying here? You have 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. you are checking for a random attribute on an index class. This is so fragile, with no documentation whatsoever. Simply make it a function call, I am not sure why this is that hard. 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.
Uhm... |
||
if self._info_axis._is_dotable and name in self._info_axis: | ||
return self[name] | ||
return object.__getattribute__(self, name) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ class NumericIndex(Index): | |
|
||
""" | ||
_is_numeric_dtype = True | ||
_is_dotable = False # Can't contain Python identifiers | ||
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. note: DatetimeIndex and friends inherit from Int64Index (which inherits from here). |
||
|
||
def __new__(cls, data=None, dtype=None, copy=False, name=None, | ||
fastpath=False): | ||
|
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.
It's getattr instead of getattribute I think