PERF: improve access of .array
State: closed

xref https://github.com/pandas-dev/pandas/issues/30790

This improves the performance of `.array` access considerably (and thus of `extract_array`, which eg caused the slowdown in the indexing benchmark). The speed-up depends on the dtype (the numpy dtypes are still quite slow ..).

This touches quite some code, but I *think* this is OK for 1.0.0 (although I mixed a bit of clean-up into it, I can also remove that again). 
@jbrockmendel I went the way of adding "yet another array/values like thing", but only on the Block (`Block.array_values()`). I think it might be possible to let `_values` return a DatetimeArray instead of datetime64 ndarray for datetime-like data, but that's something that needs more investigation, and also, even if possible, not something I would like to do on 1.0.0, but only for 1.1.

On master:
```
In [1]: s = pd.Series(pd.date_range("2012-01-01", periods=3, tz='UTC')) 

In [3]: %timeit s.array   
17.2 µs ± 219 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [4]: s = pd.Series(pd.date_range("2012-01-01", periods=3)) 

In [6]: %timeit s.array   
4.3 µs ± 205 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [7]: s = pd.Series([1, 2, 3]) 

In [9]: %timeit s.array 
6.76 µs ± 208 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
```

With this PR:

```
In [1]: s = pd.Series(pd.date_range("2012-01-01", periods=3, tz='UTC'))  

In [3]: %timeit s.array 
290 ns ± 6.26 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [4]: s = pd.Series(pd.date_range("2012-01-01", periods=3))   

In [6]: %timeit s.array 
809 ns ± 7.05 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [7]: s = pd.Series([1, 2, 3])   

In [9]: %timeit s.array 
2.34 µs ± 93.7 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
```

Labels:
name: Performance
description: Memory or execution speed performance

Comments:
> I think it might be possible to let _values return a DatetimeArray instead of datetime64 ndarray for datetime-like data, but that's something that needs more investigation,

I've been looking at something similar.  If we do the same for TimedeltaArray, this would allow us to clean up a lot of code that we currently use for boxing dt64/td64 ndarrays (com.maybe_box_datetimelike, libindex.convert_scalar).  So generally +1 on this idea in a separate PR.
I generally like what's going on here, though as Joris mentioned the proliferation of foo_value is not ideal.  

It would help if the cleanups could be separated out to limit the scope.

If the motivation is the performance of `extract_array`, then I think a much less invasive solution would be to call `obj._values` rather than `obj.array`.  Slightly different behavior, but I think a large majority of use cases use `extract_numpy=True` anyway.
> If the motivation is the performance of extract_array, then I think a much less invasive solution would be to call obj._values rather than obj.array

It's also the performance of `.array` itself, I would say. But for `extract_array`, that means we would still need to do a dtype check, as `_values` gives datetime64/timedelta64 and needs to be converted in DatetimeArray/TimedeltaArray.
>  But for extract_array, that means we would still need to do a dtype check, as _values gives datetime64/timedelta64 and needs to be converted in DatetimeArray/TimedeltaArray.

To retain the current behavior yes, but the original intention for extract_array didnt do that casting, it was just un-wrapping whatever we already had.  It was pretty specifically a replacement for the `getattr(values, "_values", getattr(values, "values", ...))` patterns

That said, I _do_ like the idea of making Series._values return DTA/TDA for instead of ndarray (or if necessary a different name), as that would allow de-duplicating a bunch of code.
> To retain the current behavior yes,

But changing that behaviour is not something I want to do for 1.0 I think. So if that is the goal, are you OK with the approach in this PR?
> But changing that behaviour is not something I want to do for 1.0 I think. So if that is the goal, are you OK with the approach in this PR?

I think all the relevant methods are private so we retain the option to revisit in the future, so this works for me.
Updated this to not include the cleaning part. What's left is just for improving `.array` performance, with limited changes.
Thanks Joris, the changes look good.

Can you add a comment somewhere documenting the differences between `.array`, `array_values`, `_values`?
> Can you add a comment somewhere documenting the differences between .array, array_values, _values?

Can you indicate what you find missing in the docs? See also https://github.com/pandas-dev/pandas/pull/31103 (I split that off from here on request) which included the docstring updates (eg I updated `Index._values` docstring, and clarified internal/external_values in the blocks)
What if we override Block.internal_values for DatetimeBlock and TimedeltaBlock to return DTA/TDA in 1D cases?  i.e. block.internal_values would behave like block.array_values does in this PR ATM, but without adding a new foo_values.

IIUC the main thing this would effect would be Series._values by making it match Index._values, which I view as desirable.
> See also #31103 (I split that off from here on request) which included the docstring updates

Thanks, that looks good.
> What if we override Block.internal_values for DatetimeBlock and TimedeltaBlock to return DTA/TDA in 1D cases? i.e. block.internal_values would behave like block.array_values does in this PR ATM, but without adding a new foo_values.
>
> IIUC the main thing this would effect would be Series._values by making it match Index._values, which I view as desirable.

@jbrockmendel Isn't that in practice the same as what we discussed before on using `_values` and change that to return DTA/TDA? To which I said: I think that would be a nice clean-up, but not something I want to do for 1.0.
> > What if we override Block.internal_values for DatetimeBlock and TimedeltaBlock to return DTA/TDA in 1D cases? i.e. block.internal_values would behave like block.array_values does in this PR ATM, but without adding a new foo_values.
> > IIUC the main thing this would effect would be Series._values by making it match Index._values, which I view as desirable.
> 
> @jbrockmendel Isn't that in practice the same as what we discussed before on using `_values` and change that to return DTA/TDA? To which I said: I think that would be a nice clean-up, but not something I want to do for 1.0.

let’s make a function to do this then; having 4 properties to do very similar things is so confusing


> let’s make a function to do this then; having 4 properties to do very similar things is so confusing

Can you be more specific in what you are suggesting? 
There are 3 properties related to this PR: `values`, `_values` and `array`, and those are obviously not going away. 
Currently, those are implemented by dispatching each of those properties to an equivalent method on the Block (well, 2 already did it this way, and in this PR I make the third (`array`) consistent with this). For sure, there are other ways to implement this instead of dispatching to the Block. But such a refactor is out of scope for this PR, I think. In this PR, I looked for a minimal change, consistent with the code we already have, to fix the performance issue for 1.0.
I tried out a branch where I made Block.internal_values return TDA/DTA for TimedeltaBlock/DatetimeBlock.  What I learned:

- a 1-line patch is needed in core.apply to prevent passing TDA/DTA to cython fastpath
- a 3-line patch is needed in IndexOpsMixin.unique to keep the return type unchanged
- `s.array` is slightly more performant than this PR for the tz-naive dt64 case, less performant in the other two cases (though still better than master, especially for the tzaware case)

```
import pandas as pd
from pandas.core.construction import extract_array

s1 = pd.Series(pd.date_range("2012-01-01", periods=3, tz='UTC'))
s2 = pd.Series(pd.date_range("2012-01-01", periods=3))
s3 = pd.Series([1, 2, 3])

%timeit s1.array
21.9 µs ± 706 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)    <-- master
738 ns ± 10.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  <-- _values branch
357 ns                                                                      <-- #31037 (est)

%timeit s2.array
5.39 µs ± 155 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)    <-- master
783 ns ± 22.6 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)   <-- _values branch
998 ns                                                                       <-- #31037 (est)

%timeit s3.array
7.94 µs ± 145 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)  <-- master
5.81 µs ± 224 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)  <-- _values branch
2.88 µs                                                                    <-- #31037 (est)

%timeit extract_array(s1, extract_numpy=True)
23.2 µs ± 371 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)     <-- master
1.32 µs ± 13.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  <-- _values branch

%timeit extract_array(s2, extract_numpy=True)
6.73 µs ± 136 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)    <-- master
1.38 µs ± 13.4 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  <-- _values branch

%timeit extract_array(s3, extract_numpy=True)
10.3 µs ± 252 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)    <-- master
1.33 µs ± 9.33 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  <-- _values branch

%timeit extract_array(s3, extract_numpy=False)
8.89 µs ± 93.1 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)  <-- master
7.49 µs ± 276 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)   <-- _values branch
```
> > let’s make a function to do this then; having 4 properties to do very similar things is so confusing
> 
> Can you be more specific in what you are suggesting?
> There are 3 properties related to this PR: `values`, `_values` and `array`, and those are obviously not going away.
> Currently, those are implemented by dispatching each of those properties to an equivalent method on the Block (well, 2 already did it this way, and in this PR I make the third (`array`) consistent with this). For sure, there are other ways to implement this instead of dispatching to the Block. But such a refactor is out of scope for this PR, I think. In this PR, I looked for a minimal change, consistent with the code we already have, to fix the performance issue for 1.0.

I don't think this patch is worth the added complexity in 1.0.0; happy to entertain in 1.1, even there having now *4* internal properties is just too much. need to find a better way.
> I don't think this patch is worth the added complexity in 1.0.0; happy to entertain in 1.1, even there having now 4 internal properties is just too much. need to find a better way.

If you have a proposal for a simpler way to fix the performance regression, that's very welcome, I am all ears. But I think this PR is really not that complex: it's not adding more properties, it's using similar patterns as we already use, and it's only a 40-lines diff.
@jbrockmendel thanks Brock, that looks interesting! As I said, I agree that's the good long term road (making `_values` to return DTA/TDA), but I still think that is a more risky change that would be better for 1.1 so we have more time to have it in master (but we can still do that also after this PR?)
I'd be on board with moving forward with this approach for 1.0 and then swapping in the _values approach for 1.1
> > I don't think this patch is worth the added complexity in 1.0.0; happy to entertain in 1.1, even there having now 4 internal properties is just too much. need to find a better way.
> 
> If you have a proposal for a simpler way to fix the performance regression, that's very welcome, I am all ears. But I think this PR is really not that complex: it's not adding more properties, it's using similar patterns as we already use, and it's only a 40-lines diff.

don't create yet another routine, array_values. any other way is way better.

I don't think this is worth jamming in 1.0.0, unless its much simpler. (LOC) is not a good measure here.
BTW, just updated the _values branch with the PandasDType.name change in this branch and that improved the s3.array and extract_values(s3) perf quite a bit (enough to match this branch in s3.array).  That two-line change should be uncontroversial for 1.0.
> BTW, just updated the _values branch with the PandasDType.name change in this branch and that improved the s3.array and extract_values(s3) perf quite a bit (enough to match this branch in s3.array). That two-line change should be uncontroversial for 1.0.

is that in a PR?
> BTW, just updated the _values branch with the PandasDType.name change in this branch and that improved the s3.array and extract_values(s3) perf quite a bit (enough to match this branch in s3.array). That two-line change should be uncontroversial for 1.0.

is that in a PR?
> is that in a PR?

Just a local branch so far, posted timeit comparisons above.
@TomAugspurger @jbrockmendel any remaining comment?

> any other way is way better.

@jreback Do you have a concrete proposal for a "any other way" ? I can't think of one (except the `_values` idea that's being discussed with @jbrockmendel that can be done as a follow-up)
> @TomAugspurger @jbrockmendel any remaining comment?
> 
> > any other way is way better.
> 
> @jreback Do you have a concrete proposal for a "any other way" ? I can't think of one (except the `_values` idea that's being discussed with @jbrockmendel that can be done as a follow-up)

your proposal is rejected as too complex - this is pure technical debt; you are free to propose other things; you are the PR author

either close it or propose for 1.1 (or even 1.0.1) if it adds less complexity

jamming this in at the last minute is not helpful 





We have time. I don’t think we’re releasing 1.0.0 today and likely not this week.

Will take a closer look tomorrow. 
Regardless the PandasDtype.name thing should be done.  @jorisvandenbossche any idea what portion of the perf bump we get from that?
> Regardless the PandasDtype.name thing should be done. @jorisvandenbossche any idea what portion of the perf bump we get from that?

happy for that. also your _values branch @jbrockmendel could be a substitute for this.
> Regardless the PandasDtype.name thing should be done. @jorisvandenbossche any idea what portion of the perf bump we get from that?

See https://github.com/pandas-dev/pandas/pull/31037#discussion_r367020643. There I timed this to be around 1.8µs (on a total of around 8µs). So it is a smaller, but significant part (but only for pandas arrays of course)