BUG: Fix some PeriodIndex resampling issues
State: closed

closes #15944 
xref partially #12884
closes #13083
closes #13224

This PR addresses some of the issues related to PeriodIndex resampling.
As I'm new to the pandas codebase, I appreciate any advice.

Labels:
name: Period
description: Period data type
name: Resample
description: resample method

Comments:
lmk when you need review. 
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=h1) Report
> Merging [#16153](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/ad7d051bdfdbadc4221307d691fd55412d9d7ae8?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `97.61%`.

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

```diff
@@            Coverage Diff             @@
##           master   #16153      +/-   ##
==========================================
- Coverage   91.27%   91.23%   -0.05%     
==========================================
  Files         163      163              
  Lines       49765    49774       +9     
==========================================
- Hits        45421    45409      -12     
- Misses       4344     4365      +21
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.02% <97.61%> (-0.03%)` | :arrow_down: |
| #single | `40.33% <7.14%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/resample.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzYW1wbGUucHk=) | `96.05% <97.61%> (-0.12%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.38% <0%> (-1.82%)` | :arrow_down: |
| [pandas/compat/numpy/function.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9udW1weS9mdW5jdGlvbi5weQ==) | `93.12% <0%> (-0.21%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.73% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/sparse/array.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2FycmF5LnB5) | `91.58% <0%> (-0.01%)` | :arrow_down: |
| [pandas/core/indexes/interval.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9pbnRlcnZhbC5weQ==) | `92.85% <0%> (ø)` | :arrow_up: |
| [pandas/core/tools/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/16153?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvZGF0ZXRpbWVzLnB5) | `83.79% <0%> (+0.82%)` | :arrow_up: |

------

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

ok, gave parametrized tests a shot in 68f02f7.
In my opinion, those are much more readable - it's clear at first sight which combinations of parameters/inputs are tested.

If you agree, I'd be happy to help moving/refactoring more of the tests in TestPeriodIndex. I'd like to tackle some more issues with PeriodIndex in a follow-up PR (```convention``` parameter, maybe drop ```kind``` parameter altogether, ...) which would require better coverage of all parameter combinations/edge cases.
Rebased on current master, so we can put the parametrized test in the existing test classes (which just inherit from ```object```).
Thanks for feedback, all requested changed should be completed.
cc @MaximilianR can you have a look.
Looks great. Fixes a couple of important issues. 

Whether or not we reveal `kind` on resamples is an open question - I think there already enough tools for switching between DateTimeIndex and PeriodIndex that we don't need more in `resample`. 

But this definitely makes things better! Thanks @winklerand 
> Whether or not we reveal kind on resamples is an open question - I think there already enough tools for switching between DateTimeIndex and PeriodIndex that we don't need more in resample.

For now this is fine. Yes I think we should remove this as PI becomes more flexible. But that's for another time.


@winklerand biggest thing is going to need a small section in 0.21.0 to show these changes. In fact I find that writing these examples forces organization on describing the actual changes. Then (in this issue or followup), ideally like to example the timeseries.rst docs w.r.t. PI resampling.
@jreback sorry for the delay on this one.

I think I made the requested changes, PTAL. Thanks for the great suggestions.

I put a small PeriodIndex resampling paragraph into the v0.21 whatsnew documentation, in the 'Backward incompatible API changes' section - is this the right place?

Going to organise the issues (closing the solved ones, open new one with the remaining issues) and create a test_resample reorg issue later tonight.
@jreback : Were we waiting for anything on this one?
side issue, maybe we should split up test_resample.py similar to groupby, e.g. make a sub-dir with common, test_datetime, test_period, test_timedelta. to make this a bit more understandable (let's make a separate issue for that).
can you rebase. this looked really good. need to review once more.
@winklerand sorry let this linger. can you rebase.
ok I rebased, will look after passing
@MaximilianR can you have a look. in particular can you look at the original issues and see if we are fully / partially covering and make a note.
This is superb, thank you @winklerand ! Much better functionality, and v nice & readable code & tests.

I still think the `kind=` kwarg is adds complication and reduces clarity, but that's for another day / the move to `IntervalIndex`.

Great work!
thanks @winklerand nice fixes!
thanks @jreback for pushing this one over the finish line, completely forgot about it (works for me syndrome ;-)), sorry!