Skip to content
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: fillna method kwd #53496

Merged
merged 14 commits into from
Jun 7, 2023
Merged

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel requested a review from rhshadrach as a code owner June 1, 2023 23:12
Comment on lines +855 to +856
.. deprecated:: 2.1.0
Use obj.ffill or obj.bfill instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're going to deprecate the method argument, we should just deprecate all of fillna here; using value with groupby provides no benefit over using it in e.g. DataFrame.fillna. I was already leaning this way while I was working on #53438.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will update.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you be OK with just doing the method deprecation here and a separate PR to deprecate the rest of the method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea - sounds good.

@@ -823,6 +823,14 @@ def fillna(self, method, limit: int | None = None):
2018-01-01 01:30:00 6.0 5
2018-01-01 02:00:00 6.0 5
"""
if method is not None:
Copy link
Member

@rhshadrach rhshadrach Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike other fillna methods, there is no value argument here, so we'd deprecate the entire method instead. But there is also a nearest option which you can't get with ffill/bfill. Does it make sense to add an nfill?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does interpolate do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit different; fillna does not fill existing NA values, only those created by upsampling.

ser = pd.Series(
    data=[1, np.nan, 3],
    index=[
        dt.datetime(2023, 3, 1, 7, 0, 0),
        dt.datetime(2023, 3, 1, 7, 0, 2),
        dt.datetime(2023, 3, 1, 7, 0, 4),
    ]
)

print(ser.resample("1s").interpolate("nearest"))
# 2023-03-01 07:00:00    1.0
# 2023-03-01 07:00:01    1.0
# 2023-03-01 07:00:02    1.0
# 2023-03-01 07:00:03    3.0
# 2023-03-01 07:00:04    3.0
# Freq: S, dtype: float64

print(ser.resample("1s").fillna(method="nearest"))
# 2023-03-01 07:00:00    1.0
# 2023-03-01 07:00:01    NaN
# 2023-03-01 07:00:02    NaN
# 2023-03-01 07:00:03    3.0
# 2023-03-01 07:00:04    3.0
# Freq: S, dtype: float64

This does seem to me to be useful when upsampling on a time series with irregular intervals.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you've looked at this more closely than i have. what's your suggestion here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, there is already a .nearest! We're good here; deprecating the entire method doesn't lose any functionality.

@mroeschke mroeschke added the Deprecate Functionality to remove in pandas label Jun 2, 2023
@rhshadrach rhshadrach mentioned this pull request Jun 3, 2023
5 tasks
warnings.warn(
f"{type(self).__name__}.fillna is deprecated and will be removed "
"in a future version. Use obj.ffill(), obj.bfill(), "
"or obj.neaerest() instead.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: nearest

@jbrockmendel jbrockmendel requested a review from mroeschke as a code owner June 7, 2023 16:45
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rhshadrach rhshadrach added the Transformations e.g. cumsum, diff, rank label Jun 7, 2023
@rhshadrach rhshadrach added this to the 2.1 milestone Jun 7, 2023
@mroeschke mroeschke merged commit eca28a3 into pandas-dev:main Jun 7, 2023
@mroeschke
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the depr-fillna-method branch June 7, 2023 23:28
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request Jul 8, 2023
* DEPR: fillna method kwd

* deprecate Resampler.fillna entirely

* suppress doctest warnings

* typo fixup

* doctest

* mypy fixup

* lint fixup
galipremsagar added a commit to rapidsai/cudf that referenced this pull request Oct 13, 2023
This PR deprecates `method` parameter in all public `fillna` APIs to match pandas: pandas-dev/pandas#53496

This PR:
```
= 1056 failed, 99098 passed, 2069 skipped, 776 xfailed, 312 xpassed, 20 errors in 670.87s (0:11:10) =
```

On `pandas_2.0_feature_branch`:
```
= 1584 failed, 98570 passed, 2069 skipped, 776 xfailed, 312 xpassed, 20 errors in 737.24s (0:12:17) =
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Transformations e.g. cumsum, diff, rank
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPR: fillna 'method'
3 participants