BUG: Fix Period and PeriodIndex support of combined offsets aliases
State: closed

- [x] closes #13730
- [x] tests added / passed
- [x] passes `git diff upstream/master | flake8 --diff`
- [x] whatsnew entry

Essentially, makes sure any `freq` string passed to a `Period` or `PeriodIndex` goes through [`_Period._maybe_convert_freq()`](https://github.com/pydata/pandas/blob/master/pandas/src/period.pyx#L682-L697), which calls [`to_offset()`](https://github.com/pydata/pandas/blob/master/pandas/tseries/frequencies.py#L389-L451), which is where the logic for combining aliases is.

All the examples in #13730 result in the correct output, and all existing tests pass. I have not written any new ones yet &mdash; I first wanted to get the opinion of the maintainers.

This PR builds on #13868 (without it, some existing tests fail).


Labels:
name: Frequency
description: DateOffsets
name: Period
description: Period data type

Comments:
this seems to be on top of #13868 was that intentional? (usually we try to do independent PR's). if they are not its ok, but then should just put in one / indicate at the top that is the case.

@sinhrks 

can you run an asv on this, make sure looks good for perf.

also would need to add tests from the issue. I dont' think we are actually testing this (and that's why it doesn't fail).

I ran the asv benchmarks. Here are the results:

```
    before     after       ratio
  [59f25575] [01c55da7]
+    3.73ms    16.80ms      4.51  reindex.frame_drop_dup_na_inplace.time_frame_drop_dup_na_inplace
+  898.72ms      3.12s      3.47  plotting.plot_andrews_curves.time_plot_andrews_curves
-   45.25ms    13.87ms      0.31  period.create_period_index_from_date_range.time_period_index
```

I have no experience whatsoever with these, so I don't know how to interpret the results above, but I find it perplexing that the two tests that run slower have absolutely nothing to do with time series...

Thanks for the test. The slowness looks to be caused by regex search. Maybe we should try dict lookup first like `get_offset` does (related to #4205, merging `to_offset` and `get_offset` to make fast/simple impl).
- https://github.com/pydata/pandas/blob/master/pandas/tseries/frequencies.py#L508

@sinhrks I'm confused. The asv test that has to do with periods is faster with this PR... 

Ah sorry, I looked first 2 ratios (looks unrelated to the period). Pls ignore my comment.

## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/13874?src=pr) is 85.31% (diff: 96.29%)

> Merging [#13874](https://codecov.io/gh/pydata/pandas/pull/13874?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will increase coverage by **<.01%**

``` diff
@@             master     #13874   diff @@
==========================================
  Files           139        139          
  Lines         50143      50146     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          42777      42780     +3   
  Misses         7366       7366          
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [7e15923...49a3783](https://codecov.io/gh/pydata/pandas/compare/7e15923d908a42008fb7fecde58be626277fd82e...49a37839a2c904fa8f0886a85f7753d392bdb94f?src=pr)

pls rebase. Need tests to close #13730 ; and add to whatsnew.

Rebased. I have also added tests for `Period`, `Period.asfreq`, `PeriodIndex`, `PeriodIndex.asfreq`, `PeriodIndex.to_datetime` and `period_range` with combined aliases.

There is one standing issue: the function [`get_standard_freq()`](https://github.com/agraboso/pandas/blob/84cc1659f8dc94ee2078b4b066551468bf5ed43e/pandas/tseries/frequencies.py#L541-L552) does not accepted combined aliases as of now. Apart from tests, it is only called from three places in the whole codebase:
- Lines [period.py#L609](https://github.com/agraboso/pandas/blob/84cc1659f8dc94ee2078b4b066551468bf5ed43e/pandas/tseries/period.py#L609) and [period.pyx#L742](https://github.com/agraboso/pandas/blob/84cc1659f8dc94ee2078b4b066551468bf5ed43e/pandas/src/period.pyx#L742) both read
  
  ```
  freqstr = frequencies.get_standard_freq(other)
  ```
  
  They can &mdash; and should &mdash; be changed to
  
  ```
  freqstr = other.rule_code
  ```
- [period.py#L488-L489](https://github.com/agraboso/pandas/blob/84cc1659f8dc94ee2078b4b066551468bf5ed43e/pandas/tseries/period.py#L488-L489)
  
  ```
  freq = Period._maybe_convert_freq(freq)
  freq = frequencies.get_standard_freq(freq)
  ```
  
  can &mdash; and should &mdash; be changed to
  
  ```
  freq = Period._maybe_convert_freq(freq).freqstr
  ```

In view of this, I propose deprecating `get_standard_freq()` (as is already done with [`get_offset_name()`](https://github.com/agraboso/pandas/blob/84cc1659f8dc94ee2078b4b066551468bf5ed43e/pandas/tseries/frequencies.py#L527-L538)) &mdash; or removing it completely if possible.

I implemented the changes and deprecation I proposed in my last comment in the last commit, including tests for the deprecation warning anywhere `get_standard_freq()` was tested.

@sinhrks Is "combined offset aliases" okay?

@agraboso Thx, it sounds ok. 

@jreback How does it look?

@jreback Done.

you are going to have a flake8 minor issue (long lines). otherwise lgtm.

can you run an asv vs master (just `-b period`) is fine, and show the results.

> you are going to have a flake8 minor issue (long lines). otherwise lgtm.

`flake8` does not complain. ~~Which are the long lines you refer to?~~ Found and fixed them.

> can you run an asv vs master (just -b period) is fine, and show the results.

```
    before     after       ratio
  [768bf495] [81262a57]
-   54.20ms    14.96ms      0.28  period.create_period_index_from_date_range.time_period_index
```

are u just showing the single change in asv?
there are more period benchmarks

Here is the whole output:

```
· Creating environments
· Discovering benchmarks
·· Uninstalling from conda-py2.7-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt.
·· Installing into conda-py2.7-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt..
· Running 24 total benchmarks (2 commits * 1 environments * 12 benchmarks)
[  0.00%] · For pandas commit hash 81262a57:
[  0.00%] ·· Building for conda-py2.7-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt....................................
[  0.00%] ·· Benchmarking conda-py2.7-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[  4.17%] ··· Running gil.nogil_datetime_fields.time_datetime_to_period                                                                                                    18.29ms
[  8.33%] ··· Running gil.nogil_datetime_fields.time_period_to_datetime                                                                                                    55.16ms
[ 12.50%] ··· Running period.create_period_index_from_date_range.time_period_index                                                                                         14.96ms
[ 16.67%] ··· Running period.period_algorithm.time_period_index_drop_duplicates                                                                                            30.93μs
[ 20.83%] ··· Running period.period_algorithm.time_period_index_value_counts                                                                                                1.12ms
[ 25.00%] ··· Running period.period_algorithm.time_period_series_drop_duplicates                                                                                           16.60ms
[ 29.17%] ··· Running period.period_algorithm.time_period_series_value_counts                                                                                               6.94ms
[ 33.33%] ··· Running period.period_setitem.time_period_setitem                                                                                                             failed
[ 37.50%] ··· Running plotting.plot_timeseries_period.time_plot_timeseries_period                                                                                         308.25ms
[ 41.67%] ··· Running timeseries.timeseries_iter_periodindex.time_timeseries_iter_periodindex                                                                                6.57s
[ 45.83%] ··· Running timeseries.timeseries_iter_periodindex_preexit.time_timeseries_iter_periodindex_preexit                                                              55.71ms
[ 50.00%] ··· Running timeseries.timeseries_period_downsample_mean.time_timeseries_period_downsample_mean                                                                  26.72ms
[ 50.00%] · For pandas commit hash 768bf495:
[ 50.00%] ·· Building for conda-py2.7-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt....
[ 50.00%] ·· Benchmarking conda-py2.7-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 54.17%] ··· Running gil.nogil_datetime_fields.time_datetime_to_period                                                                                                    19.20ms
[ 58.33%] ··· Running gil.nogil_datetime_fields.time_period_to_datetime                                                                                                    51.12ms
[ 62.50%] ··· Running period.create_period_index_from_date_range.time_period_index                                                                                         54.20ms
[ 66.67%] ··· Running period.period_algorithm.time_period_index_drop_duplicates                                                                                            33.60μs
[ 70.83%] ··· Running period.period_algorithm.time_period_index_value_counts                                                                                                1.07ms
[ 75.00%] ··· Running period.period_algorithm.time_period_series_drop_duplicates                                                                                           16.89ms
[ 79.17%] ··· Running period.period_algorithm.time_period_series_value_counts                                                                                               6.42ms
[ 83.33%] ··· Running period.period_setitem.time_period_setitem                                                                                                             failed
[ 87.50%] ··· Running plotting.plot_timeseries_period.time_plot_timeseries_period                                                                                         294.17ms
[ 91.67%] ··· Running timeseries.timeseries_iter_periodindex.time_timeseries_iter_periodindex                                                                                4.01s
[ 95.83%] ··· Running timeseries.timeseries_iter_periodindex_preexit.time_timeseries_iter_periodindex_preexit                                                              40.75ms
[100.00%] ··· Running timeseries.timeseries_period_downsample_mean.time_timeseries_period_downsample_mean                                                                  24.24ms
    before     after       ratio
  [768bf495] [81262a57]
-   54.20ms    14.96ms      0.28  period.create_period_index_from_date_range.time_period_index

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

```

By the way, I just realized that `period.period_setitem.time_period_setitem` fails even on `master`.

@jreback

thanks!