PERF: Faster Series.__getattribute__
State: closed

Closes #19764

Labels:
name: Datetime
description: Datetime data dtype
name: Performance
description: Memory or execution speed performance

Comments:
Basic idea is to let the index class say whether it can hold valid python identifiers. Classes that can

1. Index
2. MultiIndex

I think the rest can't.
Note that it's *can* contain valid identifiers, not that it *does*.

I'd be happy to hear alternative names to `is_dotable`. I was just thinking `.<key>` accessing pronounced as "dot key". 
Hmm the issue with names like `getattr*` is that it describes why it might be useful, but not what it actually does... How about `_can_hold_identifiers`?
Hello @TomAugspurger! Thanks for updating the PR.



















Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on May 01, 2018 at 00:10 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=h1) Report
> Merging [#20834](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6cacdde5630c593999059833b516e1fec60aaf72?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20834/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #20834      +/-   ##
==========================================
+ Coverage   91.77%   91.78%   +<.01%     
==========================================
  Files         153      153              
  Lines       49280    49340      +60     
==========================================
+ Hits        45229    45288      +59     
- Misses       4051     4052       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.18% <100%> (ø)` | :arrow_up: |
| #single | `41.94% <100%> (+0.06%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.94% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.64% <100%> (ø)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `84.59% <0%> (-0.21%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.15% <0%> (-0.07%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.73% <0%> (-0.04%)` | :arrow_down: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `83.95% <0%> (ø)` | :arrow_up: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `100% <0%> (ø)` | :arrow_up: |
| [pandas/api/extensions/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2FwaS9leHRlbnNpb25zL19faW5pdF9fLnB5) | `100% <0%> (ø)` | :arrow_up: |
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `92.41% <0%> (ø)` | :arrow_up: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `95.59% <0%> (+0.01%)` | :arrow_up: |
| ... and [9 more](https://codecov.io/gh/pandas-dev/pandas/pull/20834/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=footer). Last update [6cacdde...1b598d0](https://codecov.io/gh/pandas-dev/pandas/pull/20834?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

\+1 on `_can_hold_identifiers`, much better than my suggestions :-)
@jreback could you clarify your proposed solution? I don't see how

```python
NDFrame._info_axis_can_hold_identifiers()
```

is any clearer than just checking the attribute off the info axis. Why add the extra layer of indirection to just look up a piece of static data?
Oh dang, I apparently misunderstood how getattr works (this is on master).


```python
In [1]: import pandas as pd

In [2]: s = pd.Series(1, index=pd.date_range('2017', periods=10))

In [3]: getattr(s, '2017-01-01')
Out[3]: 1

```

On this branch that would raise. Ugh....
Obviously doing that is an awful idea (right? Am I missing a valid use case?), so I'm probably OK just breaking it...
Yeah, I am totally fine with breaking that
Might have missed some conversations and I realize this is wrapped up in https://github.com/pandas-dev/pandas/issues/19764#issuecomment-366757707 but have we considered dropping support for accessing index elements of a Series by name altogether? IMO seems strange to support and fraught with peril. Perhaps a little less dangerous than the previous `.ix` usage because its more limited in scope but not that far off
Allen Downey had a use case for it here:
https://github.com/AllenDowney/ModSimPy/blob/master/code/chap05.ipynb

On Fri, Apr 27, 2018 at 4:48 PM, William Ayd <notifications@github.com>
wrote:

> Might have missed some conversations and I realize this is wrapped up in #19764
> (comment)
> <https://github.com/pandas-dev/pandas/issues/19764#issuecomment-366757707>
> but have we considered dropping support for accessing index elements of a
> Series by name altogether? IMO seems strange to support and fraught with
> peril. Perhaps a little less dangerous than the previous .ix usage
> because its more limited in scope but not that far off
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20834#issuecomment-385102840>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHInMw8wt16RluF2YLgVgxtVDz9TsPks5ts5JGgaJpZM4TnRnQ>
> .
>

How is that use case any different from doing `index.loc['S']` in his examples? I'll admit up front that I'm biased against dot-notation but I find there to be a ton of nuances to supporting this that make it tough to simply convey how and when it actually works:

```python
# Don't start with anything but a letter or underscore
>>> ser = pd.Series(range(3), index=['1S', '1I', '1R'])
>>> ser.1S
SyntaxError: invalid syntax

# Don't use reserved words
>>> ser = pd.Series(range(3), index=['and', 'or', 'for'])
>>> ser.1S
SyntaxError: invalid syntax

# Hope this doesn't ever mangle
ser = pd.Series(range(3), index=['index', 'values', 'unique'])
```

Non-ASCII names would not be supported in Python2.7 (though I suppose that shouldn't dictate pandas approach at this point). As you probably alluded to in your usage of getattr above too I'm not sure how / if it should be resolving something like the below:

```python
>>> ser = pd.Series(1, index=pd.date_range('2017-01-01', periods=3, freq='s'))
>>> getattr(ser, '2017-01-01')
2017-01-01 00:00:00    1
2017-01-01 00:00:01    1
2017-01-01 00:00:02    1
Freq: S, dtype: int64
```

Don't mean to hijack this thread and this discussion could probably be moved elsewhere but figured I'd throw it out there as a future deprecation candidate

No worries about hijacking.

Allen's case is compelling, because he's using building a higher-level
interface on top of pandas. If your data only ever has these three known
fields with known names, then doing `.key` seems natural.

It'd be good to have broad feedback before deprecating this.

On Fri, Apr 27, 2018 at 5:16 PM, William Ayd <notifications@github.com>
wrote:

> How is that use case any different from doing index.loc['S'] in his
> examples? I'll admit up front that I'm biased against dot-notation but I
> find there to be a ton of nuances to supporting this that make it tough to
> simply convey how and when it actually works:
>
> # Don't start with anything but a letter or underscore>>> ser = pd.Series(range(3), index=['1S', '1I', '1R'])>>> ser.1SSyntaxError: invalid syntax
> # Don't use reserved words>>> ser = pd.Series(range(3), index=['and', 'or', 'for'])>>> ser.1SSyntaxError: invalid syntax
> # Hope this doesn't ever mangle
> ser = pd.Series(range(3), index=['index', 'values', 'unique'])
>
> Non-ASCII names would not be supported in Python2.7 (though I suppose that
> shouldn't dictate pandas approach at this point). As you probably alluded
> to in your usage of getattr above too I'm not sure how / if it should be
> resolving something like the below:
>
> >>> ser = pd.Series(1, index=pd.date_range('2017-01-01', periods=3, freq='s'))>>> getattr(ser, '2017-01-01')2017-01-01 00:00:00    12017-01-01 00:00:01    12017-01-01 00:00:02    1
> Freq: S, dtype: int64
>
> Don't mean to hijack this thread and this discussion could probably be
> moved elsewhere but figured I'd throw it out there as a future deprecation
> candidate
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20834#issuecomment-385108045>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHInPrEdvzXNi6ldioVviuCjlJ9mjDks5ts5jBgaJpZM4TnRnQ>
> .
>

how is that case compelling? why is this not using indexing under the hood (eg .loc[‘beta’]), rather relying on a attribute access which has these pitfalls?

the UX could certainly provide direct attribute access but that is not a pandas issue / problem
Use case where I, somewhat unknowingly (as you don't directly interact with the "Series" object in this case), used this in the past is with `apply` on the rows:

```
df.apply(lambda row: Point(row.x, row.y), axis=1)
```

For sure, the above can also be written with `[]` (or actually more performantly by not converting each row into a Series). 
But, I think we are for sure going to break a lot of code, for what benefit? Accessing columns as attributes also has its known pitfalls.

Anyhow, let's discuss that somewhere else if we want to continue this discussion.

For the actual PR, any objections for merging this? 

I think the main question we need to answer if we are OK with breaking `getattr(s, 'invalid_identifier')` (so something that would never work as `s.invalid_identifier`) ? 
I am fine with that.

Just calling repr(s) doesn't hit the slow code.

```python
In [6]: %timeit self.time_series_datetimeindex_repr()
1.21 s ± 57.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
```

vs.

```
In [8]: %timeit self.time_series_datetimeindex_repr()
2.08 µs ± 45.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
```
Laying out the motivation for e0710f3:

1. There's no logic in `Series.__getattr__`. All of the logic about whether `name` could possibly be in `Series.index`, so that `Series.foo` could succeed, is on the index class, where it belongs
2. I used a property so that this is easier to document, and navigate between child and parent classes than a class attribute
3. I overrode `_can_hold_identifiers` on child classes, since having the parent make that decision for the child seems improper.
4. I used a property rather than a function, since the result doesn't depend on `name`. We're trying to determine whether `series.foo` could *possibly* be in `series.index`, not whether it *is* in Series.index.
@jreback any thoughts on https://github.com/pandas-dev/pandas/pull/20834#issuecomment-385387106? Does the reason for not using a function make sense?
let me look
No rush. Won't merge without your OK.

On Mon, Apr 30, 2018 at 1:05 PM, Jeff Reback <notifications@github.com>
wrote:

> let me look
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20834#issuecomment-385480620>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIkDZ4bgdPNIVYjdBV_sZg9S0Etnyks5tt1J5gaJpZM4TnRnQ>
> .
>

> . used a property rather than a function, since the result doesn't depend on name. We're trying to determine whether series.foo could possibly be in series.index, not whether it is in Series.index.

this is simply not True. you have a comparison

```
if (self._info_axis._can_hold_identifiers and
+                    name in self._info_axis):
```

you are checking a property on the index and then do a ``__contains__`` check on the same index.


Right, I'm talking about the `_can_hold_identifiers` bit. `name in self._info_axis` is working properly.

You would replace that with something like `if self._info_axis._can_hold_identifiers_and_holds_name`?

```
def _can_hold_identifiers_and_holds_name(self, name):
     # dispatch on the type of index we are
     # no real need to subclass as this check can be confined to 
     # a small number of indexes
     if self.is_object() or self.is_categorical():
        return name in self
     return False
```

Why do two things in the function?

But at this point, I really, really am over this change. I'm just going to implement that and be done with it :)
lgtm. (assume it passes). 
Thanks Tom!
Hello, 

`pd.__version__ 0.24.2`

Is this problem related to this work? 

Very slow

    %prun df.loc['2018-11-20']


    1367 function calls (1353 primitive calls) in 12.332 seconds

    Ordered by: internal time

    ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    2   12.330    6.165   12.330    6.165 {method 'get_loc' of 'pandas._libs.index.DatetimeEngine' objects}


Very fast

    %prun  df.loc['2018-11-20':'2018-11-20']

         1199 function calls (1192 primitive calls) in 0.001 seconds

       Ordered by: internal time

       ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        6    0.000    0.000    0.000    0.000 offsets.py:2308(delta)
        4    0.000    0.000    0.000    0.000 {method 'get_loc' of 'pandas._libs.index.DatetimeEngine' objects}
        2    0.000    0.000    0.000    0.000 {pandas._libs.tslibs.parsing.parse_time_string}
That's a different issue. This was about attribute access, not indexing.

On Mon, May 6, 2019 at 11:21 AM douglasmacdonald <notifications@github.com>
wrote:

> Hello,
>
> pd.__version__ 0.24.2
>
> Is this problem related to this work?
>
> Very slow
>
> %prun df.loc['2018-11-20']
>
>
> 1367 function calls (1353 primitive calls) in 12.332 seconds
>
> Ordered by: internal time
>
> ncalls  tottime  percall  cumtime  percall filename:lineno(function)
> 2   12.330    6.165   12.330    6.165 {method 'get_loc' of 'pandas._libs.index.DatetimeEngine' objects}
>
> Very fast
>
> %prun  df.loc['2018-11-20':'2018-11-20']
>
>      1199 function calls (1192 primitive calls) in 0.001 seconds
>
>    Ordered by: internal time
>
>    ncalls  tottime  percall  cumtime  percall filename:lineno(function)
>     6    0.000    0.000    0.000    0.000 offsets.py:2308(delta)
>     4    0.000    0.000    0.000    0.000 {method 'get_loc' of 'pandas._libs.index.DatetimeEngine' objects}
>     2    0.000    0.000    0.000    0.000 {pandas._libs.tslibs.parsing.parse_time_string}
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20834#issuecomment-489681833>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIVQJAYUZXIJF6TBNLLPUBLJ5ANCNFSM4E45DHIA>
> .
>