Skip to content

BUG: make pct_change can handle the anchored freq #28664 #28681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Nov 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a6b1a68
BUG: make pct_change can handle the anchored freq #28664
dongho-jung Sep 30, 2019
5eaa983
black pandas/tests/series/test_timeseries.py
dongho-jung Sep 30, 2019
9565b40
add whatsnew entry
dongho-jung Sep 30, 2019
8295022
make its own test about pct_change with anchored freq
dongho-jung Oct 1, 2019
e7579a8
make the test more exhaustive
dongho-jung Oct 1, 2019
910adb3
fix n as 1 because offset that is more than 1 won't anchored
dongho-jung Oct 1, 2019
4c6433b
make parametrizing idiomatic, add issue number
dongho-jung Oct 2, 2019
1c77e59
remove .isAnchored logic
dongho-jung Oct 2, 2019
64043b2
revert pct_change changes
dongho-jung Oct 2, 2019
f61e62a
take a new approach, dropping duplicate indices
dongho-jung Oct 2, 2019
1e89f71
Merge remote-tracking branch 'upstream/master' into fix-GH28664
dongho-jung Oct 14, 2019
92301ff
add more test cases
andrew-buzzni Oct 18, 2019
d4fbf04
Merge remote-tracking branch 'upstream/master' into fix-GH28664
andrew-buzzni Oct 18, 2019
fadc4fe
Merge remote-tracking branch 'upstream/master' into fix-GH28664
andrew-buzzni Nov 8, 2019
d38fff5
replace the test with a sanity test
andrew-buzzni Nov 8, 2019
0c9dd60
enhance the test
andrew-buzzni Nov 8, 2019
bdbd5fd
move a whatsnew entry from Other to Reshaping
andrew-buzzni Nov 9, 2019
6ec771c
Merge remote-tracking branch 'upstream/master' into fix-GH28664
andrew-buzzni Nov 9, 2019
bd5673d
Update doc/source/whatsnew/v1.0.0.rst
dongho-jung Nov 9, 2019
147ca3c
Merge remote-tracking branch 'upstream/master' into fix-GH28664
dongho-jung Nov 9, 2019
8e12959
move a whatsnew entry from Other to Reshaping
dongho-jung Nov 9, 2019
3d70ebe
make the test more explicit
dongho-jung Nov 10, 2019
5fb77cd
increase the periods from 3 to 5
dongho-jung Nov 10, 2019
ceac393
Merge remote-tracking branch 'upstream/master' into fix-GH28664
dongho-jung Nov 10, 2019
78e86fc
Merge remote-tracking branch 'upstream/master' into fix-GH28664
andrew-buzzni Nov 14, 2019
0b0f5c0
Merge branch 'fix-GH28664' of https://github.com/0xF4D3C0D3/pandas in…
andrew-buzzni Nov 14, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ Reshaping
- :func:`qcut` and :func:`cut` now handle boolean input (:issue:`20303`)
- Fix to ensure all int dtypes can be used in :func:`merge_asof` when using a tolerance value. Previously every non-int64 type would raise an erroneous ``MergeError`` (:issue:`28870`).
- Better error message in :func:`get_dummies` when `columns` isn't a list-like value (:issue:`28383`)
- Bug :meth:`Series.pct_change` where supplying an anchored frequency would throw a ValueError (:issue:`28664`)

Sparse
^^^^^^
Expand Down
1 change: 1 addition & 0 deletions pandas/core/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -10443,6 +10443,7 @@ def pct_change(self, periods=1, fill_method="pad", limit=None, freq=None, **kwar
data = self.fillna(method=fill_method, limit=limit, axis=axis)

rs = data.div(data.shift(periods=periods, freq=freq, axis=axis, **kwargs)) - 1
rs = rs.loc[~rs.index.duplicated()]
rs = rs.reindex_like(data)
if freq is None:
mask = isna(com.values_from_object(data))
Expand Down
10 changes: 10 additions & 0 deletions pandas/tests/series/test_timeseries.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,16 @@ def test_pct_change(self, datetime_series):
rs, (filled / filled.shift(freq="5D") - 1).reindex_like(filled)
)

def test_pct_change_with_duplicate_axis(self):
# GH 28664
common_idx = date_range("2019-11-14", periods=5, freq="D")
result = Series(range(5), common_idx).pct_change(freq="B")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually can you replicate the original issue here, I think BM was used. I don't mind having B as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback I had two options, the original issue used freq BM and periods 70. I thought you also want the explicit and explainable expected, however, if I used periods 70 then the expected would be quite long and hard to explain why it is so.

so two options are as follows:

  1. replicate the original issue, but omit the explanation
  2. use freq B since it's also anchored offset, and leave the explanation as it is

both options are not hard :D
please tell me which one you more prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i c, ok, then let's leave it.


# the reason that the expected should be like this is documented at PR 28681
expected = Series([np.NaN, np.inf, np.NaN, np.NaN, 3.0], common_idx)

tm.assert_series_equal(result, expected)

def test_pct_change_shift_over_nas(self):
s = Series([1.0, 1.5, np.nan, 2.5, 3.0])

Expand Down