PERF: PeriodIndex.dropna, difference, take
State: closed

Related #23083 

Besides the performance boost, this goes a long way towards making `PeriodIndex._simple_new` actually simple; avoids passing object-dtype to `_simple_new`

```
In [2]: pi_with_nas = pd.PeriodIndex(['1985Q1', 'NaT', '1985Q2'] * 1000, freq='Q')
In [3]: %timeit pi_with_nas.dropna()
The slowest run took 13.93 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 23.3 µs per loop  <-- PR
100 loops, best of 3: 4.96 ms per loop    <-- master

In [4]: pi_diff = pd.PeriodIndex(['1985Q1', 'NaT', '1985Q3'] * 1000, freq='Q')
In [5]: %timeit pi_with_nas.difference(pi_diff)
1000 loops, best of 3: 553 µs per loop  <-- PR
100 loops, best of 3: 5.29 ms per loop  <-- master

In [6]: %timeit pi_with_nas.symmetric_difference(pi_diff)
1000 loops, best of 3: 787 µs per loop  <-- PR
100 loops, best of 3: 10.3 ms per loop  <-- master
```

Labels:

Comments:
Hello @jbrockmendel! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`asv_bench/benchmarks/period.py`](https://github.com/jbrockmendel/pandas/blob/f3d332142b3272bb5764168f862cafdfdfead110/asv_bench/benchmarks/period.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/base.py`](https://github.com/jbrockmendel/pandas/blob/f3d332142b3272bb5764168f862cafdfdfead110/pandas/core/indexes/base.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/multi.py`](https://github.com/jbrockmendel/pandas/blob/f3d332142b3272bb5764168f862cafdfdfead110/pandas/core/indexes/multi.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/period.py`](https://github.com/jbrockmendel/pandas/blob/f3d332142b3272bb5764168f862cafdfdfead110/pandas/core/indexes/period.py) !

 - There are no PEP8 issues in the file [`pandas/util/testing.py`](https://github.com/jbrockmendel/pandas/blob/f3d332142b3272bb5764168f862cafdfdfead110/pandas/util/testing.py) !

 - Complete extra results for this file :

> file_to_check.py:2454:-258: W605 invalid escape sequence '\('
file_to_check.py:2454:-255: W605 invalid escape sequence '\)'
---


And the same question as on the other PR: to what extent is this conflicting with the PeriodArray PR of @TomAugspurger ? Shouldn't we rather focus on finishing that one first, and then doing some perf improvements / clean-ups afterwards?
> Shouldn't we rather focus on finishing that one first, and then doing some perf improvements

@jorisvandenbossche @TomAugspurger responding to many of joris's comments/questions (here and in #23093) in one place.

The PeriodIndex/PeriodArray constructors are a pain point in large part because its `_simple_new` is not very simple, which is why there is the lower-level `_from_ordinals`.  Why the complication in `_simple_new`?  Because the values being passed to it are sometimes i8 (what we actually want), sometimes `ndarray[object]`, and sometimes `PeriodIndex`.  The latter two cases, as it turns out, are coming from calls to `_shallow_copy`, exactly the ones this PR affects.

i.e. following this PR, we'll be able to clean up the `PeriodIndex` constructors (on top of whats in #23093)
@jbrockmendel to be clear, I fully agree with your analysis about `_simple_new`. 
But the thing is that the PeriodArray PR already solves this partly (at least for PeriodIndex, complexity is mainly moved to PeriodArray), because then, it will be much more PeriodArray that will be passed around in the Index implementation, instead of sometimes object array / sometimes ordinals. And of course, we can then still try to simplify the different constructors in PeriodArray. 
As another example, The PeriodArray currently does not yet solve the `take` issue you try to solve here, but once that PR would land, the solution for `take` would be completely different (implementing it on the PeriodArray, dispatching to that, and properly wrapping it again), and the other changes you have here might not be needed.
> And of course, we can then still try to simplify the different constructors in PeriodArray.

That's fair.  I am definitely viewing the situation as "#22862 is a big PR and simplifying things before that will make that PR smaller, review easier, etc"
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23095?src=pr&el=h1) Report
> Merging [#23095](https://codecov.io/gh/pandas-dev/pandas/pull/23095?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/c8ce3d01e9ffafc24c6f9dd568cd9eb7e42c610c?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #23095      +/-   ##
==========================================
- Coverage    92.2%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50924    50933       +9     
==========================================
+ Hits        46952    46960       +8     
- Misses       3972     3973       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.62% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.3% <52.17%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23095?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/23095/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `95.46% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/23095/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.44% <100%> (ø)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/23095/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `86.18% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23095/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.56% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23095/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.61% <0%> (-0.14%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23095?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/23095?src=pr&el=footer). Last update [c8ce3d0...62f0f1f](https://codecov.io/gh/pandas-dev/pandas/pull/23095?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Sorry, I still don't see how introducing this `_take_values` brings us closer to our goal. The PeriodArray PR will remove that again, no?
I think this idea is that they'll all just be `.values` once we have
PeriodArray / DatetimeArray.

On Fri, Oct 12, 2018 at 7:50 AM Jeff Reback <notifications@github.com>
wrote:

> *@jreback* requested changes on this pull request.
>
> I don't agree this will be short lived. This is a new notion, meaning we
> now have
> ------------------------------
>
> In pandas/core/indexes/base.py
> <https://github.com/pandas-dev/pandas/pull/23095#discussion_r224771799>:
>
> > @@ -763,6 +763,17 @@ def get_values(self):
>          """
>          return self.values
>
> +    @property
> +    def _take_values(self):
> +        """
> +        Values to use in `take` operation, suitable to being passed back to
> +        _shallow_copy/_simple_new.
> +        """
> +        if is_period_dtype(self):
> +            # Avoid casting to object-dtype
> +            return self._ndarray_values
> +        return self.values
>
> this is basically .asi8 on Indexes now, yes? Is there a reason we are not
> aligning on that? or changing .asi8 to this?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/23095#pullrequestreview-164225426>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIgYuFBTeDDuieFgaOcWO-fKYWR-cks5ukJAegaJpZM4XYCM1>
> .
>

> I don't agree this will be short lived. 

> I think this idea is that they'll all just be `.values` once we have PeriodArray / DatetimeArray.

I tend to agree with Tom on this one, but the more important point in my book is that this fixes the weird arguments being passed to PeriodIndex._shallow_copy which allows us to clean up the constructor mess, simplifying the PeriodArray transition.
if they will be .values then don’t introduce something new 
Yah, I'm beginning to agree.  Plenty of other related stuff to focus on.  Closing.