BUG fix deprecation of `limit` and `fill_method` in `pct_change` #55527

Charlie-XIAO · 2023-10-15T08:25:28Z

Towards DEPR: pct_change method/limit keyword #53491
Tests added and passed
All code checks passed
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file

I'm thinking that we should not over-complicate things by allowing obj.pct_change(). The thing is, even if users do obj.ffill/bfill().pct_change(), a warning will still be raised unless we check for NA values, for instance, #54981. I propose that we should let users use obj.ffill/bfill().pct_change(fill_method=None) to suppress the warnings, and this PR intends to give extra specific deprecation warning messages.

I haven't added test cases for it, but want to make sure if this is the right way to go. Ping @rhshadrach and @jbrockmendel who were involved in the discussion in the original issue. I'm seeing many users and libraries complaining about this might be better to get this done before the next release.

rhshadrach

Thanks for the PR, I'm guessing tests need to be updated.

rhshadrach · 2023-10-15T10:11:38Z

pandas/core/generic.py

-        if fill_method is not lib.no_default or limit is not lib.no_default:
+        # GH#53491: deprecate the `fill_method` and `limit` keyword, except
+        # `fill_method=None` that does not fill missing values
+        if fill_method not in (lib.no_default, None) and limit is not lib.no_default:


I don't think we need warning messages for every single case. Does something like

The 'fill_method' being not None and the 'limit' argument are deprecated. Either fill in NA values prior to calling pct_change or specify fill_method=None to not fill NA values.

work?

I think we may want to prompt users to use fill_method=None even if they have filled NA values. See the original comment of this PR. The case is, even after ffill or bfill, calling pct_change without keyword will raise deprecation warning unless we explicitly check if there are NA values to fill. However, I don't think this is a good approach: (1) this may add too much overhead, and (2) if a user is not filling NA values and uses pct_change without keyword, and if the data occasionally does not contain NA values, he/she will not get a warning message and the logic would be incorrect.

Due to these reasons, I think this deprecation would be especially confusing, especially since we are having "incorrect" deprecation warnings in the current version. That's why I'm trying to give extra specific guide for each case. If maintainers do not think this is necessary, I can implement using only a single message.

Still, I need a confirmation about whether we should prompt users to do obj.ffill/bfill().pct_change(fill_method=None) or obj.ffill/bfill().pct_change(). Personally I prefer the former as I explained in the previous comment.

However, I don't think this is a good approach: (1) this may add too much overhead, and (2) if a user is not filling NA values and uses pct_change without keyword, and if the data occasionally does not contain NA values, he/she will not get a warning message and the logic would be incorrect.

I'm seeing the current logic takes 7.5% of the runtime for the current warning on a Series with 100k rows - I don't think overhead is a concern. This will cause users to modify their code unnecessarily in what I think is the uncommon case. They will then need to change their code again when we deprecate the fill_method argument. I do not think we should do that.

Sure, I will implement your suggestions.

Perhaps "Either fill in any non-leading NA values" is better.

agreed with @rhshadrach

Charlie-XIAO · 2023-10-16T01:52:07Z

@rhshadrach I've implemented your suggestions, please check if it is correct. I've also updated the test cases and now they seem to pass correctly. Not sure if I need to add some additional tests?

By the way, for instance

>>> ser = pd.Series([np.nan, 1, 2, 3, np.nan])
>>> ser.bfill().pct_change()

still raises a warning. Should we fix that?

rhshadrach · 2023-10-22T13:51:35Z

/preview

github-actions · 2023-10-22T13:51:46Z

Website preview of this PR available at: https://pandas.pydata.org/preview/55527/

rhshadrach

Looks good! Small request on the tests.

pandas/tests/frame/methods/test_pct_change.py

pandas/tests/series/methods/test_pct_change.py

rhshadrach · 2023-10-22T14:02:44Z

Also - this needs a whatsnew note.

Charlie-XIAO · 2023-10-23T10:33:59Z

Done, not sure if the changelog should be added in v2.2.0 or in v2.1.2. Currently it is in v2.2.0 but let me know if this is wrong or the wording needs to be corrected.

Also, may I have your response to #55527 (comment) please @rhshadrach?

rhshadrach · 2023-10-23T19:52:42Z

From #55527 (comment):

still raises a warning. Should we fix that?

I believe the behavior is going to change between now and pandas 3.0. So that means we need to warn.

rhshadrach

Missed one request in the whatsnew, otherwise I think we're all set.

doc/source/whatsnew/v2.2.0.rst

Charlie-XIAO · 2023-10-25T17:14:01Z

Done @rhshadrach, thanks for your review!

lithomas1 · 2023-10-25T20:26:11Z

Bumping off the milestone.

rhshadrach

lgtm

doc/source/whatsnew/v2.1.2.rst

rhshadrach

lgtm

rhshadrach · 2023-10-26T09:33:10Z

Thanks @Charlie-XIAO

rhshadrach · 2023-10-26T09:34:36Z

@meeseeksdev backport 2.1.x

lumberbot-app · 2023-10-26T09:34:59Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout 2.1.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 54814c3bc022b91447c27e72b8f79cdac1f6df15

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #55527: BUG fix deprecation of `limit` and `fill_method` in `pct_change`'

Push to a named branch:

git push YOURFORK 2.1.x:auto-backport-of-pr-55527-on-2.1.x

Create a PR against branch 2.1.x, I would have named this PR:

"Backport PR #55527 on branch 2.1.x (BUG fix deprecation of limit and fill_method in pct_change)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

…l_method` in `pct_change`

#55701) Backport PR #55527: BUG fix deprecation of `limit` and `fill_method` in `pct_change` Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>

Charlie-XIAO added 4 commits October 4, 2023 15:26

extra specific deprecation messages

cc8d745

deprecation in docstring

afbeac0

Merge remote-tracking branch 'upstream/main' into redepr-pct-change

7991ac6

use fill_method=None

096bc78

Charlie-XIAO requested a review from rhshadrach as a code owner October 15, 2023 08:25

Charlie-XIAO mentioned this pull request Oct 15, 2023

DEPR: pct_change method/limit keyword #53491

Closed

rhshadrach requested changes Oct 15, 2023

View reviewed changes

Charlie-XIAO added 3 commits October 15, 2023 23:01

apply richard's suggestions

e7c6296

Merge remote-tracking branch 'upstream/main' into redepr-pct-change

80e190f

update tests correspondingly

6e457ef

mroeschke added Bug Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff labels Oct 16, 2023

rhshadrach requested changes Oct 22, 2023

View reviewed changes

pandas/tests/frame/methods/test_pct_change.py Outdated Show resolved Hide resolved

pandas/tests/series/methods/test_pct_change.py Outdated Show resolved Hide resolved

rhshadrach added this to the 2.1.2 milestone Oct 22, 2023

Charlie-XIAO added 2 commits October 23, 2023 18:22

Merge remote-tracking branch 'upstream/main' into redepr-pct-change

d874bb5

changelog added; suggestions from Richard

b18801e

rhshadrach requested changes Oct 23, 2023

View reviewed changes

doc/source/whatsnew/v2.2.0.rst Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

Charlie-XIAO added 2 commits October 26, 2023 01:08

Merge remote-tracking branch 'upstream/main' into redepr-pct-change

dc4bff1

updated changelog

33f3053

lithomas1 modified the milestones: 2.1.2, 2.1.3 Oct 25, 2023

rhshadrach approved these changes Oct 26, 2023

View reviewed changes

rhshadrach requested changes Oct 26, 2023

View reviewed changes

doc/source/whatsnew/v2.1.2.rst Outdated Show resolved Hide resolved

Update v2.1.2.rst

dd00755

rhshadrach approved these changes Oct 26, 2023

View reviewed changes

rhshadrach merged commit 54814c3 into pandas-dev:main Oct 26, 2023
30 of 33 checks passed

This comment was marked as outdated.

Sign in to view

lumberbot-app bot added the Still Needs Manual Backport label Oct 26, 2023

rhshadrach modified the milestones: 2.1.3, 2.1.2 Oct 26, 2023

rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request Oct 26, 2023

Backport PR pandas-dev#55527: BUG fix deprecation of limit and `fil…

9dfb53c

…l_method` in `pct_change`

lithomas1 removed the Still Needs Manual Backport label Oct 26, 2023

mroeschke mentioned this pull request Nov 29, 2023

Deprecate fill_method and limit in pct_change APIs rapidsai/cudf#14277

Merged

3 tasks

Charlie-XIAO deleted the redepr-pct-change branch April 15, 2024 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG fix deprecation of `limit` and `fill_method` in `pct_change` #55527

BUG fix deprecation of `limit` and `fill_method` in `pct_change` #55527

Charlie-XIAO commented Oct 15, 2023 •

edited

Loading

rhshadrach left a comment

rhshadrach Oct 15, 2023 •

edited

Loading

Charlie-XIAO Oct 15, 2023

Charlie-XIAO Oct 15, 2023 •

edited

Loading

rhshadrach Oct 15, 2023 •

edited

Loading

Charlie-XIAO Oct 15, 2023

rhshadrach Oct 15, 2023

phofl Oct 15, 2023

Charlie-XIAO commented Oct 16, 2023

rhshadrach commented Oct 22, 2023

github-actions bot commented Oct 22, 2023

rhshadrach left a comment

rhshadrach commented Oct 22, 2023

Charlie-XIAO commented Oct 23, 2023

rhshadrach commented Oct 23, 2023

rhshadrach left a comment

This comment was marked as duplicate.

Charlie-XIAO commented Oct 25, 2023

lithomas1 commented Oct 25, 2023

rhshadrach left a comment

rhshadrach left a comment

rhshadrach commented Oct 26, 2023

This comment was marked as outdated.

rhshadrach commented Oct 26, 2023

lumberbot-app bot commented Oct 26, 2023

BUG fix deprecation of limit and fill_method in pct_change #55527

BUG fix deprecation of limit and fill_method in pct_change #55527

Conversation

Charlie-XIAO commented Oct 15, 2023 • edited Loading

rhshadrach left a comment

Choose a reason for hiding this comment

rhshadrach Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

Charlie-XIAO Oct 15, 2023

Choose a reason for hiding this comment

Charlie-XIAO Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

rhshadrach Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

Charlie-XIAO Oct 15, 2023

Choose a reason for hiding this comment

rhshadrach Oct 15, 2023

Choose a reason for hiding this comment

phofl Oct 15, 2023

Choose a reason for hiding this comment

Charlie-XIAO commented Oct 16, 2023

rhshadrach commented Oct 22, 2023

github-actions bot commented Oct 22, 2023

rhshadrach left a comment

Choose a reason for hiding this comment

rhshadrach commented Oct 22, 2023

Charlie-XIAO commented Oct 23, 2023

rhshadrach commented Oct 23, 2023

rhshadrach left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Charlie-XIAO commented Oct 25, 2023

lithomas1 commented Oct 25, 2023

rhshadrach left a comment

Choose a reason for hiding this comment

rhshadrach left a comment

Choose a reason for hiding this comment

rhshadrach commented Oct 26, 2023

This comment was marked as outdated.

rhshadrach commented Oct 26, 2023

lumberbot-app bot commented Oct 26, 2023

BUG fix deprecation of `limit` and `fill_method` in `pct_change` #55527

BUG fix deprecation of `limit` and `fill_method` in `pct_change` #55527

Charlie-XIAO commented Oct 15, 2023 •

edited

Loading

rhshadrach Oct 15, 2023 •

edited

Loading

Charlie-XIAO Oct 15, 2023 •

edited

Loading

rhshadrach Oct 15, 2023 •

edited

Loading