CLN: Move some PI/DTI methods to EA subclasses, implement tests
State: closed

If/when this goes through the basic template it creates for tests will be pretty straightforward to extend to the other relevant methods/properties.

@TomAugspurger this definitely overlaps with #22862, particularly the pieces touching constructors.  LMK if this causes problems and I can try to stay in my lane.

Labels:
name: Period
description: Period data type
name: Clean

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

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/0a5f343f2bf6ef16525224582d6f6c342befc5e3/pandas/core/arrays/datetimes.py) !

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

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

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

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

 - There are no PEP8 issues in the file [`pandas/tests/arrays/test_datetimelike.py`](https://github.com/jbrockmendel/pandas/blob/0a5f343f2bf6ef16525224582d6f6c342befc5e3/pandas/tests/arrays/test_datetimelike.py) !


Updated with suggested edits.

Initial thoughts on the constructors:

- I'd like to get rid of `_from_ordinals` and have `_simple_new` be at the bottom-end of the constructor chain.

- I don't totally understand the use case for `_shallow_copy_with_infer`.  If it could be made unnecessary, I'd be +1 on that.

- -1 on adding public constructors just yet.  Harder to undo later.  But `_from_periods` seems like a reasonable method for "construct from list-like of periods".

> (what's in the dict? Currently it's attributes, but I think all those (freq for PeriodIndex) will be pushed onto PeriodArray, so maybe it isn't necessary anymore?).

That was to allow for code to be shared between PeriodArray/PeriodIndex; the PeriodIndex version would pass `name` while PeriodArray wouldn't pass any kwargs.  So if/when inheritance is removed in favor of composition, that can definitely go.

> `PeriodArray.__init__ :: (ndarray[int], Tick) -> None` (the ndarray hold ordinals)

I'd prefer to have the PeriodArray usage be more similar to the PeriodIndex usage, in which case the first argument would accept more variants (accept PeriodIndex, PeriodArray, ndarray[Period], Series[Period], ...).  Do you have a different usage in mind?
There's a certain attractiveness to having the `__init__` being just a type check and assignment to `self`. Regardless, I think it's fine to accept those for now, and when we do the Array refactor we can see how many times we're calling `PeriodArray.__init__` directly. If most of the calls go through `_simple_new` or `_from_periods` then maybe we could move the remained to go through those.
> I don't totally understand the use case for _shallow_copy_with_infer. If it could be made unnecessary, I'd be +1 on that.

The difference between `_shallow_copy_with_infer` and `_shallow_copy` is that the first one can actually return a different class, based on the values it is passed. Eg:

```
In [71]: idx = pd.Index(['a', 'b', 'c'])

In [72]: idx._shallow_copy_with_infer(np.array([1., 2.]))
Out[72]: Float64Index([1.0, 2.0], dtype='float64')

In [73]: idx._shallow_copy(np.array([1., 2.]))
Out[73]: Index([1.0, 2.0], dtype='float64')     <---- strange result
```

but both still pass through attributes (name, etc).

> _simple_new :: (Union[ndarray, ExtensionArray], Optional[name], Dict) (what's in the dict? Currently it's attributes, but I think all those (freq for PeriodIndex) will be pushed onto PeriodArray, so maybe it isn't necessary anymore?).

If you want to allow the `_simple_new` of PeriodIndex to accept ndarray, you still need to be able to pass the `freq` in the dict as well. But if we only accept PeriodArray, the dict could be dropped. But I think we still need a constructor from ndarray+freq on the index as well (unless we do `_simple_new(PeriodArray(valuues, freq))`, but that might be a bit verbose.



# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22961?src=pr&el=h1) Report
> Merging [#22961](https://codecov.io/gh/pandas-dev/pandas/pull/22961?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/4976ee1c93bcc6327ca5e8714ad326c126f4151e?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/22961/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22961?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22961      +/-   ##
==========================================
+ Coverage   92.19%   92.19%   +<.01%     
==========================================
  Files         169      169              
  Lines       50831    50845      +14     
==========================================
+ Hits        46864    46878      +14     
  Misses       3967     3967
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <100%> (ø)` | :arrow_up: |
| #single | `42.35% <29.68%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22961?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/22961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `92.8% <100%> (+0.82%)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/22961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.31% <100%> (-0.35%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.74% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `96.91% <100%> (+0.09%)` | :arrow_up: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/22961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.16% <100%> (ø)` | :arrow_up: |

------

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

Totally happy to continue the constructors discussion here.  First though: any other thoughts on the PR?  I think original comments have all been addressed.
FYI, given https://github.com/pandas-dev/pandas/pull/22992 I think there's no need to wait for circle.
Travis failure is unrelated hypothesis warning
@TomAugspurger This is fine for me, but I will defer to you for merging (so you can time it if needed with your period work)
happy to merge this (or you can).