-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
ENH: Add limit_area to ffill/bfill #56531
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.
I think we'll also need to add limit_area
as apart of the ExtensionArray._pad_or_backfill in the future as well
Thanks @rhshadrach |
@@ -857,3 +857,100 @@ def test_pad_backfill_deprecated(func): | |||
df = DataFrame({"a": [1, 2, 3]}) | |||
with tm.assert_produces_warning(FutureWarning): | |||
getattr(df, func)() | |||
|
|||
|
|||
@pytest.mark.parametrize( |
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.
im a bit late here, but these tests look really similar to the series ones. is there a viable way to share them?
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.
Yes - but where would they go? Just all in frame/methods?
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.
usually we put them in the frame/methods/test_foo.py file and parametrize over frame_or_series
# GH#56492 | ||
df = DataFrame(data) | ||
expected = DataFrame(expected_data) | ||
result = getattr(df, method)(**kwargs) |
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.
Looks like the tests all use numpy dtypes; would this work with EAs? id expect the limit_area kwd to be added to EA._pad_of_backfill but dont see it there
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 - I missed this, interpolate currently ignores limit_area with ffill/bfill and EAs, so this will too. The implementation appears to be straightforward enough to add. I think I should be able to do this in the next week, but do we want to revert this PR for the RC?
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.
im not going to lose sleep either way
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.
I think I should be able to do this in the next week, but do we want to revert this PR for the RC?
I would say it's fine to just add this after the RC
* ENH: Add limit_area to ffill/bfill * Fix bfill default * Update groupby API test * Update groupby API test
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Only adds to Series/DataFrame. Using ffill/bfill in Resampler.interpolate is also deprecated, but it appears to be a bit more work in order to implement there. I've opened #56532 to track.