-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
DEPR fill_method
and limit
keywords in pct_change
#53520
Conversation
There was a problem hiding this comment.
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 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? |
fill_method
and limit
keywords in pct_change
fill_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. |
@jbrockmendel - what's your recommended way forward here? |
|
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. |
This reverts commit 2cb449b.
fill_method
in pct_change
fill_method
and limit
keywords in pct_change
There was a problem hiding this comment.
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? |
Thanks @Charlie-XIAO |
…3520) * 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
…3520) * 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 |
Does obj.fillna(...).pct_change not work, as is also mentioned in the deprecation message? @holymonson |
We don't want |
@holymonson - I've responded in #53491 |
doc/source/whatsnew/v2.1.0.rst
file