Repository: pandas-dev/pandas
Pull request: BUG: make pct_change can handle the anchored freq #28664
- [x] closes #28664
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

    pct_change didn't work when the freq is anchored(like `1W`, `1M`, `BM`)
    so when the freq is anchored, use `data.asfreq(freq)` instead of the raw
    `data`.

Problem statement:
Bug: pct_change with frequency set as 'BM' throws value error

#### Code Sample, a copy-pastable example if possible
```
import pandas as pd
import random
import numpy as np


Creating the time-series index 
n=60
index = pd.date_range('01/13/2020', periods = 70,freq='D') 
  
Creating the dataframe  
df = pd.DataFrame({"A":np.random.uniform(low=0.5, high=13.3, size=(70,)), 
                   "B":np.random.uniform(low=10.5, high=45.3, size=(70,)),  
                   "C":np.random.uniform(low=70.5, high=85, size=(70,)), 
                   "D":np.random.uniform(low=50.5, high=65.7, size=(70,))}, index = index) 


df.pct_change(freq='BM')

```
#### Problem description

For a time-series data, the df.pct_change(freq='BM') doesn't works and throws following error.

ValueError: cannot reindex from a duplicate axis

#### Expected Output
df.asfreq('BM').pct_change()

#### Output of ``pd.show_versions()``

<details>

INSTALLED VERSIONS
------------------
commit           : None
python           : 3.6.3.final.0
python-bits      : 64
OS               : Windows
OS-release       : 10
machine          : AMD64
processor        : Intel64 Family 6 Model 142 Stepping 9, GenuineIntel
byteorder        : little
LC_ALL           : None
LANG             : None
LOCALE           : None.None

pandas           : 0.25.1
numpy            : 1.15.4
pytz             : 2017.2
dateutil         : 2.6.1
pip              : 18.0
setuptools       : 39.1.0
Cython           : 0.26.1
pytest           : 5.1.2
hypothesis       : None
sphinx           : 1.6.3
blosc            : None
feather          : None
xlsxwriter       : 1.0.2
lxml.etree       : 4.1.0
html5lib         : 0.9999999
pymysql          : None
psycopg2         : 2.7.5 (dt dec pq3 ext lo64)
jinja2           : 2.9.6
IPython          : 6.1.0
pandas_datareader: None
bs4              : 4.6.0
bottleneck       : 1.2.1
fastparquet      : None
gcsfs            : None
lxml.etree       : 4.1.0
matplotlib       : 2.1.0
numexpr          : 2.6.2
odfpy            : None
openpyxl         : 2.4.8
pandas_gbq       : None
pyarrow          : 0.8.0
pytables         : None
s3fs             : None
scipy            : 1.0.0
sqlalchemy       : 1.1.13
tables           : 3.4.2
xarray           : 0.10.7
xlrd             : 1.1.0
xlwt             : 1.3.0
xlsxwriter       : 1.0.2

</details>

Hints from issue discussion:
Hello, I have found that this happens when the offset which is made of `freq` is anchored. (`offset.isAnchored`)

so I propose that when the freq gets anchored, make the data resampled first, before the next line
`rs = data.div(data.shift(periods=periods, freq=freq, axis=axis, **kwargs)) - 1`

```diff
--- a/pandas/core/generic.py
+++ b/pandas/core/generic.py
@@ -10413,6 +10413,8 @@ class NDFrame(PandasObject, SelectionMixin):
         else:
             data = self.fillna(method=fill_method, limit=limit, axis=axis)

+        if freq and to_offset(freq).isAnchored():
+            data = data.asfreq(freq)
         rs = data.div(data.shift(periods=periods, freq=freq, axis=axis, **kwargs)) - 1
         rs = rs.reindex_like(data)
         if freq is None:
```
However, I couldn't run tests yet because, in the latest commit c4489c, it spewed out a lot of fails.
@0xF4D3C0D3 Can you change the label of this issue as Bug ?
No, I think I can't do that because I'm not a contributor nor member.
good! the tests are passed. I'll create the PR

Commit message: add whatsnew entry

Patch to review:
diff --git a/doc/source/whatsnew/v1.0.0.rst b/doc/source/whatsnew/v1.0.0.rst
index eb4b72d01d59a..fbf40d1055b40 100644
--- a/doc/source/whatsnew/v1.0.0.rst
+++ b/doc/source/whatsnew/v1.0.0.rst
@@ -301,6 +301,7 @@ Other
 - Using :meth:`DataFrame.replace` with overlapping keys in a nested dictionary will no longer raise, now matching the behavior of a flat dictionary (:issue:`27660`)
 - :meth:`DataFrame.to_csv` and :meth:`Series.to_csv` now support dicts as ``compression`` argument with key ``'method'`` being the compression method and others as additional compression options when the compression method is ``'zip'``. (:issue:`26023`)
 - :meth:`Series.append` will no longer raise a ``TypeError`` when passed a tuple of ``Series`` (:issue:`28410`)
+- :meth:`Series.pct_change` will can handle the anchored freq such as `1M`, `1W`, `BM`. (:issue:`28664`)
 
 .. _whatsnew_1000.contributors:
 
diff --git a/pandas/core/generic.py b/pandas/core/generic.py
index a3b9bec494854..569139783f5c5 100644
--- a/pandas/core/generic.py
+++ b/pandas/core/generic.py
@@ -10413,6 +10413,8 @@ def pct_change(self, periods=1, fill_method="pad", limit=None, freq=None, **kwar
         else:
             data = self.fillna(method=fill_method, limit=limit, axis=axis)
 
+        if freq and to_offset(freq).isAnchored():
+            data = data.asfreq(freq)
         rs = data.div(data.shift(periods=periods, freq=freq, axis=axis, **kwargs)) - 1
         rs = rs.reindex_like(data)
         if freq is None:
diff --git a/pandas/tests/series/test_timeseries.py b/pandas/tests/series/test_timeseries.py
index d0ca5d82c6b33..6519f5fcfdc0e 100644
--- a/pandas/tests/series/test_timeseries.py
+++ b/pandas/tests/series/test_timeseries.py
@@ -415,6 +415,10 @@ def test_pct_change(self):
             rs, (filled / filled.shift(freq="5D") - 1).reindex_like(filled)
         )
 
+        rs = self.ts.pct_change(freq="1W")
+        filled = self.ts.fillna(method="pad")
+        assert_series_equal(rs, self.ts.asfreq("1W"))
+
     def test_pct_change_shift_over_nas(self):
         s = Series([1.0, 1.5, np.nan, 2.5, 3.0])
 


Code review comments:
diff_hunk: @@ -10413,6 +10413,8 @@ def pct_change(self, periods=1, fill_method="pad", limit=None, freq=None, **kwar
         else:
             data = self.fillna(method=fill_method, limit=limit, axis=axis)
 
+        if freq and to_offset(freq).isAnchored():
path: pandas/core/generic.py
text: @user1:
Could just be my lack of familiarity but what does `isAnchored` do here that would need to differ from the non-anchored case?

@author:
first, the `isAnchored` isn't documented well. you can find it [here](https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.tseries.offsets.DateOffset.isAnchored.html#pandas.tseries.offsets.DateOffset.isAnchored). I just found it by digging into the code.

I got to know that pct_change didn't work when the freq is anchored so I added that logic.

@user2:
so shift already does this, but .div does not (that's why you get the error); if we *always* do an ``.asfreq()`` do tests still pass?

@author:
Oh, then I think I have about 2 options. one is that fix `.div` or another one is making the test more meticulously. I gotta try both!

@author:
@user2  as you pointed out, I didn't cover all cases like follow.
![image](https://user-images.githubusercontent.com/19762154/65970134-ed617c00-e4a0-11e9-9145-bf22e05b00c4.png).

And I have made tests more exhaustive now.

![image](https://user-images.githubusercontent.com/19762154/65970344-547f3080-e4a1-11e9-95af-b5805d3cf0dc.png)

Next, I'll look into `.div` :D

@user2:
that's not what I meant. .div is not freq aware (nor should it).

pls don't post pictures, just code

@author:
ok, I'm sorry about that. And you're right the .div is under the `_flex_method_SERIES` and I think I can't do something about it. I'll verify the `.asfreq` approach

@user1:
So I think the outstanding question here is if the `to_offset(...` piece is required - is that something you took a look at @user3 ?

@author:
I thought that handling the anchored freq was a key though, after the several testing, I have found that the approach I took was wrong.

so I take a new approach, dropping duplicate indices before reindexing.
I made a new review at the bottom.