Skip to content
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.

Sign up for GitHub

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

PERF: Faster Series.__getattribute__ #20834

Merged
merged 7 commits into from
May 1, 2018
Merged

PERF: Faster Series.__getattribute__ #20834

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ad6a202
PERF: Faster Series.__getattribute__
TomAugspurger Apr 26, 2018
55072a0
Renamed
TomAugspurger Apr 26, 2018
ad871ce
Linting
TomAugspurger Apr 26, 2018
4b7334c
Fixed find-replace fail
TomAugspurger Apr 26, 2018
857364f
asv
TomAugspurger Apr 28, 2018
e0710f3
Move to instance properties
TomAugspurger Apr 30, 2018
1b598d0
function
TomAugspurger May 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.23.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,7 @@ Performance Improvements
- Improved performance of :func:`pandas.core.groupby.GroupBy.any` and :func:`pandas.core.groupby.GroupBy.all` (:issue:`15435`)
- Improved performance of :func:`pandas.core.groupby.GroupBy.pct_change` (:issue:`19165`)
- Improved performance of :func:`Series.isin` in the case of categorical dtypes (:issue:`20003`)
- Improved performance of ``getattr(Series, attr)`` when the Series has certain index types. This manifiested in slow printing of large Series with a ``DatetimeIndex`` (:issue:`19764`)
- Fixed a performance regression for :func:`GroupBy.nth` and :func:`GroupBy.last` with some object columns (:issue:`19283`)

.. _whatsnew_0230.docs:
Expand Down
3 changes: 2 additions & 1 deletion pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4375,7 +4375,8 @@ def __getattr__(self, name):
name in self._accessors):
return object.__getattribute__(self, name)
else:
if name in self._info_axis:
Copy link
Contributor

Choose a reason for hiding this comment

The 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 __contains__, e.g. a string in a DTI. This check can be non-trivial because the index hashtable may need to be created to test if its a valid method.

Instead I would create a _contains private method to do this, where by it must be an exact dtype (and not a possible one, like a string in a DTI).

Note we already have contains method so can't really use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. a string in a DTI

These can't be valid identifiers though, so they by definition can't be going through __getatrr__.

We can perform this cheap check at the class level, without having to deal with values at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future folks will have no idea this is called

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my point is that you instead should be calling a function like

if self._info_axis._._can_hold(name):
     return self[name]

putting this here buries all kinds of logic in an obscure routine. no-one will be able to understand / find this later.
These should be the simplest routines possible, while pushing logic to the axis object itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it be a function? To outcome doesn't depend on name.

putting this here buries all kinds of logic in an obscure routine

Can you say exactly what logic you think I'm burying here? You have if self._info_axis._can_hold(name). I have if self._info_axis._can_hold_identifiers.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this is that hard.

Uhm...

if (self._info_axis._can_hold_identifiers and
name in self._info_axis):
return self[name]
return object.__getattribute__(self, name)

Expand Down
6 changes: 6 additions & 0 deletions pandas/core/indexes/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,12 @@ class Index(IndexOpsMixin, PandasObject):
_engine_type = libindex.ObjectEngine

_accessors = set(['str'])
# Whether items can be selected from NDFrame.<item>
# Some indexes (DatetimeIndex, Int64Index) cannot contain
# valid Python identifiers. Setting _can_hold_identifiers = False is an
# optimization.
# https://github.com/pandas-dev/pandas/issues/19764
_can_hold_identifiers = True

str = CachedAccessor("str", StringMethods)

Expand Down
1 change: 1 addition & 0 deletions pandas/core/indexes/interval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ class IntervalIndex(IntervalMixin, Index):
_typ = 'intervalindex'
_comparables = ['name']
_attributes = ['name', 'closed']
_can_hold_identifiers = False # can't contain Python identifiers

# we would like our indexing holder to defer to us
_defer_to_indexing = True
Expand Down
1 change: 1 addition & 0 deletions pandas/core/indexes/numeric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class NumericIndex(Index):

"""
_is_numeric_dtype = True
_can_hold_identifiers = False # Can't contain Python identifiers

def __new__(cls, data=None, dtype=None, copy=False, name=None,
fastpath=False):
Expand Down
1 change: 1 addition & 0 deletions pandas/core/indexes/period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ class PeriodIndex(DatelikeOps, DatetimeIndexOpsMixin, Int64Index):
"""
_typ = 'periodindex'
_attributes = ['name', 'freq']
_can_hold_identifiers = False # Can't contain Python identifiers

# define my properties & methods for delegation
_other_ops = []
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/indexes/datetimelike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

class DatetimeLike(Base):

def test_can_hold_identifiers(self):
idx = self.create_index()
assert idx._can_hold_identifiers is False

def test_shift_identity(self):

idx = self.create_index()
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/indexes/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ def generate_index_types(self, skip_index_keys=[]):
if key not in skip_index_keys:
yield key, idx

def test_can_hold_identifiers(self):
idx = self.create_index()
assert idx._can_hold_identifiers is True

def test_new_axis(self):
new_index = self.dateIndex[None, :]
assert new_index.ndim == 2
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/indexes/test_category.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ def create_index(self, categories=None, ordered=False):
return CategoricalIndex(
list('aabbca'), categories=categories, ordered=ordered)

def test_can_hold_identifiers(self):
ci = self.create_index(categories=list('abcd'))
assert ci._can_hold_identifiers is True

def test_construction(self):

ci = self.create_index(categories=list('abcd'))
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/indexes/test_multi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ def setup_method(self, method):
def create_index(self):
return self.index

def test_can_hold_identifiers(self):
idx = self.create_index()
assert idx._can_hold_identifiers is True

def test_boolean_context_compat2(self):

# boolean context compat
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/indexes/test_numeric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ def test_index_rdiv_timedelta(self, scalar_td, index):

class Numeric(Base):

def test_can_hold_identifiers(self):
idx = self.create_index()
assert idx._can_hold_identifiers is False

def test_numeric_compat(self):
pass # override Base method

Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/indexes/test_range.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ def check_binop(self, ops, scalars, idxs):
expected = op(Int64Index(idx), scalar)
tm.assert_index_equal(result, expected)

def test_can_hold_identifiers(self):
idx = self.create_index()
assert idx._can_hold_identifiers is False

def test_binops(self):
ops = [operator.add, operator.sub, operator.mul, operator.floordiv,
operator.truediv]
Expand Down