clean up PeriodIndex constructor
State: closed

- [x] closes #13232 
- [x] tests added / passed
- [x] passes `git diff upstream/master | flake8 --diff`
- [x] whatsnew entry

Material clean up of PeriodIndex constructor, which was doing a few weird things (https://github.com/pydata/pandas/issues/13232#issuecomment-220788816), and generally getting messy.


Labels:
name: Period
description: Period data type
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
@jreback One of the reasons the `PeriodIndex` constructer is so convoluted is there are some odd cases supported (I imagine through many bugfixes & iterations!). One of them is a _list of ints representing years_. An array of ints are interpreted as ordinals, but a list is coerced to years within the `Period` constructor.

Here's an example: https://github.com/SixtyCapital/pandas/blob/master/pandas/tseries/tests/test_period.py#L2216

What do you think about not allowing that?

@MaximilianR I think that's convenient. The issue is you need a constructor that only deals with codes (otherwise its ambiguous), ok; let's rename `_from_arraylike` to `from_codes` and make it public; that will solve the ambiguity problem then (and also provide the internal mechanism for creation which is THE way to create from codes).

OK, but then you have these being different results: 

```
pd.PeriodIndex([2007, 2008], freq='A')  # years
pd.PeriodIndex(np.array([2007, 2008]), freq='A')  # ordinals 
pd.PeriodIndex(np.array([2007, 2008], dtype='object'), freq='A')  # ?
```

Agree on separation of concerns there - I have `._simple_new` handling ordinals, and `__new__` handling the other cases now.

But we still need `pd.PeriodIndex(idx.values, idx.freq)` to treat the values as ordinals rather than years (otherwise would break a fundamental tenet of indexes). So we're left with the above. 
I can add back the branching logic for lists vs. arrays if you really think it's a helpful use case?

hmm, those should all do the same thing.

> hmm, those should all do the same thing.

Right. So we can either allow supplying years as ints OR those all the same thing: choose one!
Or a creative solution?

actually looking closer I don't think integers are EVER interpreted as ordinals directly, instead the integers are intrepreted according to the frequency. Can you give a counter-example?

hmm, maybe that's not true. but in any event an example would help.

> actually looking closer I don't think integers are EVER interpreted as ordinals directly, 

From some test failures earlier, but without much exploration on its frequency): in some indexing operations, `PeriodIndex(some_values_from_a_periodindex)` is called. 
There may be ways around that _but_ are you sure we want to violate the `idx._constructor(idx.values) == idx` consistency?

> instead the integers are intrepreted according to the frequency.

You need the freq to convert an ordinal to a Period, for sure

> `idx._constructor(idx.values) == idx`

`idx.values` is an implementation detail. I have no problem with forcing US (meaning you!) to change internal access methods to `PeriodIndex.from_ordinals(idx.values)` if you want to construct from ordinals and leave `PeriodIndex([2000,2001],freq='A-JUNE')` to work as the public contstructor.

IOW separating out ordinal construction.

OK, let me reflect a bit. 

My current thinking depends what you mean by 'internal access methods' - absolutely fine if that's _within_ the PeriodIndex machinery. My fear is that there's a lot of pandas that relies on assertion above, and that going down this road would make `PeriodIndex` need _more_ special casing throughout the library. And I would love to see less, both for `PeriodIndex` and everything else!

One option would be to move towards the Index interface relying on `._simple_new` to interpret any output of the values of an Index. I think that probably isn't a big change.

> My current thinking depends what you mean by 'internal access methods- absolutely fine if that's _within_ the PeriodIndex machinery. My fear is that there's a lot of pandas that relies on assertion above, and that going down this road would makePeriodIndexneed _more_ special casing throughout the library. And I would love to see less, both forPeriodIndex` and everything else!

You mean that people are actually constructing this like `PeriodIndex(idx.values)`? and expecting it to work? in external code, really? 

If you can fix up the internal consistency, does anything actually break? (e.g. create `PeriodIndex.from_ordinals` then move all internal code to use that). 

> You mean that people are actually constructing this like PeriodIndex(idx.values)? and expecting it to work? in external code, really?

No! But there are places in pandas this happens outside of the PeriodIndex class. I need to turn this off and see what breaks - I suspect a reasonable amount but not a torrent

@MaximilianR ok does look a lot cleaner

I've been remiss about the final fixes here. Will need a couple of weeks on other commitments but I will get to this

Unless @sinhrks is going to spoil all my hard work with moving PeriodIndex to be a proper type

I'm using `PeriodIndex` as internal of `PeriodDtype` (like `DatetimeTZ`), the improvement of `PeriodIndex` should not be spoiled:)

@MaximilianR do you have time to update this? (the period dtype for index PR has been merged) Then we could get it in 0.19

I'll have a look in the next couple of weeks. I fear the underlying may have changed enough that this is no longer valid / needed, but let me have a look and assess

@MaximilianR can you update

Yes, apologies for my slowness with this + a couple of others. I'll have a plug over / after Thanksgiving. Thanks for the ping

@MaximilianR can you rebase
Just tried rebasing. Painful. Good lesson for getting smaller changes in faster. Will attempt to resolve
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/13277?src=pr&el=h1) Report
> Merging [#13277](https://codecov.io/gh/pandas-dev/pandas/pull/13277?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/04e116851337cd852b4255f8221d9be44829e0e1?src=pr&el=desc) will **decrease** coverage by `-0.03%`.
> The diff coverage is `97.95%`.


```diff
@@            Coverage Diff             @@
##           master   #13277      +/-   ##
==========================================
- Coverage   91.07%   91.04%   -0.03%     
==========================================
  Files         136      136              
  Lines       49165    49157       -8     
==========================================
- Hits        44775    44756      -19     
- Misses       4390     4401      +11
```


| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/13277?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.48% <100%> (ø)` | :white_check_mark: |
| [pandas/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL2luZGV4ZXMvYmFzZS5weQ==) | `96.17% <100%> (ø)` | :white_check_mark: |
| [pandas/io/packers.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhY2tlcnMucHk=) | `87.57% <100%> (ø)` | :white_check_mark: |
| [pandas/tseries/period.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGVyaW9kLnB5) | `92.67% <97.67%> (-0.13%)` | :x: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :x: |
| [pandas/tools/merge.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL3Rvb2xzL21lcmdlLnB5) | `91.78% <0%> (-0.35%)` | :x: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.83% <0%> (-0.1%)` | :x: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/13277?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/13277?src=pr&el=footer). Last update [04e1168...5cae7aa](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...5cae7aaebe4ce5272720e4c07e1828e67a6a4c44?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
@jreback green!
🤸‍♀️ 
@MaximilianR looks pretty good. just a change about the unpickling constructor.
@MaximilianR Can you run some of the asv benchmarks to check there is no impact on performance? (also the plotting benchmarks, we have see regressions there in the past due to changes in period index)
@jorisvandenbossche Good call. There is one significant slow down, by about 30x: [`time_from_pydatetime`](https://github.com/SixtyCapital/pandas/blob/period-float/asv_bench/benchmarks/period.py#L15), which cooerces python datetimes to Periods.

I've tried to debug this, but am struggling a bit and could do with some help (and general guidance on how to approach these problems). So far:
- The `Period` constructor is being called by each object within `period.extract_ordinals`, and that's what's taking all the time
- The `Period` constructor then calls [`get_period_ordinal`](https://github.com/pandas-dev/pandas/blob/master/pandas/src/period_helper.c#L1151), which somewhere calls into `to_offset`:
```python
 0.183 __new__  pandas/tseries/period.py:183
├─ 0.168 wrapper  pandas/util/decorators.py:65
│  └─ 0.150 to_offset  pandas/tseries/frequencies.py:453
│     ├─ 0.089 get_stride_from_decimal  pandas/tseries/frequencies.py:186
│     │  └─ 0.086 isclose  numpy/core/numeric.py:2375
│     │     ├─ 0.032 within_tol  numpy/core/numeric.py:2434
│     │     └─ 0.021 all  numpy/core/fromnumeric.py:1987
│     │        ├─ 0.013 _all  numpy/core/_methods.py:40
│     │        └─ 0.003 asanyarray  numpy/core/numeric.py:484
│     ├─ 0.019 get_offset  pandas/tseries/frequencies.py:605
│     │  └─ 0.014 copy  pandas/tseries/offsets.py:306
│     │     └─ 0.013 __init__  pandas/tseries/offsets.py:192
│     ├─ 0.014 __mul__  pandas/tseries/offsets.py:412
│     │  └─ 0.006 __init__  pandas/tseries/offsets.py:192
│     │     └─ 0.004 _determine_offset  pandas/tseries/offsets.py:198
│     │        └─ 0.003 <genexpr>  pandas/tseries/offsets.py:203
│     └─ 0.008 split  re.py:195
└─ 0.014 get_freq_code  pandas/tseries/frequencies.py:300
   └─ 0.003 _period_str_to_code  pandas/tseries/frequencies.py:732
```

Apart from exactly how the code traces through:
- I'm not sure how _these changes_ should make an impact on this code path 
- Does this matter? Coercing from python datetime isn't a common use case


CC @sinhrks 
I think I figured it out - `Period._maybe_convert_freq` needs to be called early, otherwise it the pyx functions will take a slow route to get the appropriate freq. Running ASV to confirm now
Performance issues solved! I think all all asv changes are from external compute interference given none are period related. Here are all <50% or >200% for transparency:

```
-  112.12ms    54.32ms      0.48  sparse.sparse_frame_constructor.time_sparse_frame_constructor
-    7.34ms     3.48ms      0.47  reshape.reshape_stack_simple.time_reshape_stack_simple
-    5.97ms     2.70ms      0.45  reshape.melt_dataframe.time_melt_dataframe
-    1.00ms   448.65μs      0.45  reindex.FillMethod.time_pad
-    1.87ms   825.54μs      0.44  period.Algorithms.time_value_counts_pindex
-    1.15ms   485.69μs      0.42  reindex.Reindexing.time_reindex_dates
-  429.95μs   173.53μs      0.40  series_methods.series_constructor_no_data_datetime_index.time_series_constructor_no_data_datetime_index
-   24.72ms     4.49ms      0.18  reindex.Duplicates.time_frame_drop_dups_na_inplace
-   18.22ms     2.74ms      0.15  reindex.Reindexing.time_reindex_multiindex

---

+    6.13ms    14.80ms      2.41  hdfstore_bench.HDF5.time_read_store_mixed
+   26.90ms    60.93ms      2.27  groupby.GroupBySuite.time_rank('int', 100)
+    1.48ms     3.23ms      2.19  frame_methods.Interpolate.time_interpolate_some_good
+  537.00μs     1.16ms      2.16  strings.StringEncode.time_encode_decode
+    2.62ms     5.43ms      2.07  strings.StringMethods.time_contains_few_noregex
``` 
@jreback green!
couple of comments. lgtm otherwise. if you'd fix / create a new issue (for the handling of datetime-aware tz, other datetimes), then ok with merging.
can you rebase / update