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
Deprecate DataFrame.fillna
/Series.fillna
with method
#10349
Deprecate DataFrame.fillna
/Series.fillna
with method
#10349
Conversation
fillna
with 'method' is deprecatedDataFrame.fillna
/Series.fillna
with method
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.
small comment, otherwise looks generally good to me.
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.
Overall, the code looks good. I have one open question/suggestion around test readability and one potential new issue we should file.
return parts.map_overlap( | ||
M.fillna, before, after, method=method, limit=limit, meta=meta | ||
) | ||
return self.bfill(limit=limit, axis=axis) |
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.
Potential follow-up: It looks like we're not validating method
anywhere and simply default to bfill
even in the case of typos. If I'm correct, we should probably handle bfill
in an elif
and raise on a "default" else
.
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 preserves the previous behavior:
if method in ("pad", "ffill"):
...
else:
method = "bfill"
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 comment is out of scope for this PR, it was more of a general remark about the existing behavior that we may want to fix at some point.
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.
Thanks, @j-bennet!
See #10347.
Handles
fillna
deprecations like this one:pre-commit run --all-files
The remaining failures are not the same issue.