DEPR fill_method and limit keywords in pct_change by Charlie-XIAO · Pull Request #53520 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits16 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- Closes DEPR: pct_change method/limit keyword #53491
- Tests added and passed
- All code checks passed
- Added an entry in the latest
doc/source/whatsnew/v2.1.0.rstfile
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be good if the default for fill_method was None. However, it is currently "pad". When the deprecation is enforced, I think we'll want the behavior to be None instead. This means we first need to deprecate/change the result, and then once that's done we can deprecate the argument.
Can these be done in a single PR or in separate PRs?
I don't think these can be done in a single major version. In 2.x we deprecate the default; in 3.x we can then change the default to None. Finally in 3.x we can also deprecate the arguments and in 4.x we can remove them.
@rhshadrach I have updated the PR to deprecate only the default value of fill_method. Not sure if I'm doing correctly, so please let me know if I should make changes.
It also seems that deprecating this behavior is raising warnings in so many tests. I've already fixed some of those, do I need to fix the rest as well (like what I'm currently doing), or is there a better resolution?
DEPR DEPR default value of fill_method and limit keywords in pct_changefill_method in pct_change
I disagree with @rhshadrach that this needs to be a two-major-version process. This is just not that big of a deal.
- warn if method is explicitly passed
- otherwise warn if there are NAs present
Ah - thanks; that sounds great to me.
Okay, so in other words, revert to my original version, and in addition warn when has NA but method is not explicit passed.
I will make the changes soon. Thanks for your discussion.
DEPR default value of DEPR fill_method in pct_changefill_method and limit keywords in pct_change
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Could you merge main once more?
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request
DEPR fill_method and limit keywords in DataFrame/Series pct_change
DEPR fill_method and limit keywords in GroupBy pct_change
changelog added
DataFrame/Series pct_change only deprecate default
typo
GroupBy pct_change only deprecate default
changelod updated correspondingly
reverting
Revert "reverting"
This reverts commit 2cb449b.
resolved conversation: also warn if nan and not specified
modify msgs since fillna method deprecated; tests updated
changelog updated
docstring use ffill instead of fillna
@rhshadrach @jbrockmendel Hi, we need fill_method='None' to force pct_change not to handle NAs, but this PR cause a lot of deprecated warning and we can't fix it. Before you provide an alternative to do that, deprecatingfill_method is unwise.
Does obj.fillna(...).pct_change not work, as is also mentioned in the deprecation message? @holymonson
Does obj.fillna(...).pct_change not work, as is also mentioned in the deprecation message? @holymonson
We don't want fillna() at all, on the contrary we want keeping NAs, while your deprecation message doesn't tell howto NOT fillna.