ENH: Deprecate non-keyword arguments for Resampler.interpolate #41699

Anupam-USP · 2021-05-28T12:59:07Z

xref Deprecate non-keyword arguments for methods with inplace #41485
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

pep8speaks · 2021-05-28T13:52:50Z

Hello @Anupam-USP! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-04 16:15:25 UTC

MarcoGorelli

I'll review later, but note that this should be Resampler.interpolate, not DataFrame.interpolate - example usage

Anupam-USP · 2021-05-28T14:55:57Z

I'll review later, but note that this should be Resampler.interpolate, not DataFrame.interpolate - example usage

I have tried but this pre-commit test is not passing by any means. What should I do for passing it?

MarcoGorelli · 2021-05-28T15:03:37Z

pandas/tests/resample/test_deprecated.py

+def test_interpolate_posargs_deprecation():
+
+    s = pd.Series([0.0, 1.0, np.nan, 3.0])
+
+    msg = (
+        r"In a future version of pandas all arguments of DataFrame\.interpolate "
+        r"except for the argument 'method' will be keyword-only"
+    )
+
+    with tm.assert_produces_warning(FutureWarning, match=msg):
+        result = s.interpolate()
+
+    expected = pd.Series([0.0, 1.0, 2.0, 3.0])


there needs to be resample somewhere in here, and the index should be datetimes, see the example in this question https://stackoverflow.com/q/47148446/4451315 and the other tests in this file

MarcoGorelli · 2021-05-28T15:04:33Z

I'll review later, but note that this should be Resampler.interpolate, not DataFrame.interpolate - example usage

I have tried but this pre-commit test is not passing by any means. What should I do for passing it?

see https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#pre-commit

MarcoGorelli

Getting there 💪

Just a few comments - also, could you add a test where you call resample on a Series?

pandas/tests/resample/test_deprecated.py

MarcoGorelli · 2021-05-28T19:27:36Z

pandas/tests/resample/test_deprecated.py

+    data = StringIO(
+        """\
+    Values
+    1992-08-27 07:46:48,1
+    1992-08-27 07:46:59,4"""
+    )
+    s = pd.read_csv(data, squeeze=True)


can you construct this directly? something like

data = DataFrame({'ds': ['1992-08-27 07:46:48, '1992-08-27 07:46:59'], 'y': [1, 4]})

MarcoGorelli · 2021-05-28T19:27:50Z

pandas/tests/resample/test_deprecated.py

+    data_exp = StringIO(
+        """\
+    Values
+    1992-08-27 07:46:48,1.0
+    1992-08-27 07:46:51,1.0
+    1992-08-27 07:46:54,1.0
+    1992-08-27 07:46:57,1.0"""
+    )
+
+    expected = pd.read_csv(data_exp, squeeze=True)


likewise, construct this directly

Okay, will do it in some time!

…nterpolate

Anupam-USP · 2021-05-29T06:19:33Z

I always fail some cases, no matter what I do. Pasting the check's error is also not giving satisfactory results. What should I do to pass them all?

MarcoGorelli · 2021-05-29T08:27:45Z

pandas/tests/resample/test_deprecated.py

+    msg = (
+        r"In a future version of pandas all arguments of DataFrame\.interpolate "
+        r"except for the argument 'method' will be keyword-only"
+    )


Does this test pass? The message doesn't seem right, and I"d expect it to be different for the Series case

Have you run

pytest pandas/tests/resample/test_deprecated.py

locally?

It is giving me following error AssertionError: Did not see expected warning of class 'FutureWarning'

OK, so that needs fixing first :) I suggest you remove the with tm.assert_produces_warning, dedent the following line, run the test, check what error message you get, and use that

Done that, got new issues due to resample("3s"), trying to resolve it. I think I have to create expected in another way.

MarcoGorelli · 2021-05-29T08:28:48Z

pandas/tests/resample/test_deprecated.py

+    expected = Series(
+        df.iloc[:, 1].values.reshape(-1), index=pd.to_datetime(df.iloc[:, 0].values)
+    )


construct expected explicitly, don't put logic in tests

MarcoGorelli · 2021-05-29T08:29:21Z

pandas/tests/resample/test_deprecated.py

+    with tm.assert_produces_warning(FutureWarning, match=msg):
+        result = s.resample("3s").interpolate("linear")


need to construct expected and check that result matches it, tm.assert_series_equal

MarcoGorelli · 2021-05-29T08:31:52Z

pandas/tests/resample/test_deprecated.py

+    df = DataFrame({"ds": ["1992-08-27 07:46:48", "1992-08-27 07:46:59"], "y": [1, 4]})
+    s = Series(
+        df.iloc[:, 1].values.reshape(-1), index=pd.to_datetime(df.iloc[:, 0].values)
+    )


Is there a simpler way to create s?

I think it is needed to give this logic for s, otherwise it is taking the indexes making it not operable

can you do something like

idx = pd.to_datetime(["1992-08-27 07:46:48", "1992-08-27 07:46:59"]) df = DataFrame({'y': [1, 4]}, index=idx) ser = Series([1, 4], index=idx)

MarcoGorelli · 2021-05-30T09:01:56Z

pandas/tests/resample/test_deprecated.py

+    msg = (
+        r"In a future version of pandas all arguments of DataFrame\.interpolate "
+        r"except for the argument 'method' will be keyword-only"
+    )


OK, so that needs fixing first :) I suggest you remove the with tm.assert_produces_warning, dedent the following line, run the test, check what error message you get, and use that

MarcoGorelli · 2021-05-30T13:08:33Z

pandas/tests/resample/test_deprecated.py

+        df.iloc[:, 1].values.reshape(-1), index=pd.to_datetime(df.iloc[:, 0].values)
+    )
+
+    result = s.resample("3s").interpolate("linear")


Once you've got the warning message, you need to put the

with tm.assert_produces_warning

back

But with that it is not able to pass the testcase

then you need to fix the code so it passes the testcase 😄

jreback · 2021-05-31T16:27:08Z

@Anupam-USP if you can rebase & update

Anupam-USP · 2021-06-02T06:22:36Z

I am not able to get rid of this issue AssertionError: Did not see expected warning of class 'FutureWarning' when I am using with tm.assert_produces_warning. Sorry I tried!

MarcoGorelli · 2021-06-02T07:42:44Z

I am not able to get rid of this issue AssertionError: Did not see expected warning of class 'FutureWarning' when I am using with tm.assert_produces_warning. Sorry I tried!

What happens if you remove the tm.assert_produces_warning line and run the test? Can you show your complete output?

Anupam-USP · 2021-06-02T07:52:25Z

This happens when i insert the test, otherwise checks are getting passed without it

MarcoGorelli · 2021-06-02T07:53:30Z

otherwise checks are getting passed without it

Do they still pass if you also change

result = s.resample("3s").interpolate("linear")

to

result = s.resample("3s").interpolate("linear", 0)

?
(you're deprecating axis as positional, but not method). Then, this should raise some warning

If you want, let's continue this discussion on gitter https://gitter.im/pydata/pandas

Anupam-USP · 2021-06-02T07:54:27Z

No means when I am removing it, I am not getting any error. It comes only after inserting it

MarcoGorelli · 2021-06-02T07:57:27Z

So, without the tm.asssert_produces_warning line, and with

result = s.resample("3s").interpolate("linear", 0)

instead of

result = s.resample("3s").interpolate("linear")

, it passes? OK, I'll take a look on Friday

Anupam-USP · 2021-06-02T08:02:06Z

Yes it got passed. Thank you, but I wanted to know how? And it isn't mentioned anywhere clearly?

…nterpolate

MarcoGorelli

deprecate_nonkeyword_arguments will raise a warning if any argument is passed as positional (except for method). If you pass axis as positional, it'll raise a warning - that's why the test now passes. If this isn't documented clearly in deprecate_nonkeyword_arguments, then feel free to open a PR to clarify it

Could you address https://github.com/pandas-dev/pandas/pull/41699/files#r641915415 and https://github.com/pandas-dev/pandas/pull/41699/files#r641909565 please?

MarcoGorelli · 2021-06-04T10:06:59Z

pandas/tests/resample/test_deprecated.py

+    )
+    expected = Series([1.0, 1.0, 1.0, 1.0], index=idx)
+
+    expected.index.freq = "3s"


@jbrockmendel in other tests I've seen you do

expected.index._data.freq = "3s"

is that preferrable here, or is this OK?

the ._data.freq = is preferable

pandas/tests/resample/test_deprecated.py

MarcoGorelli · 2021-06-05T09:12:49Z

Thanks @Anupam-USP

…s-dev#41699) * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * ENH: Deprecate non-keyword arguments for interpolate * Update pandas/tests/resample/test_deprecated.py Co-authored-by: Marco Edward Gorelli <[email protected]>

Anupam-USP added 2 commits May 28, 2021 18:27

ENH: Deprecate non-keyword arguments for interpolate

638255f

ENH: Deprecate non-keyword arguments for interpolate

fc93316

Anupam-USP added 2 commits May 28, 2021 19:28

ENH: Deprecate non-keyword arguments for interpolate

c526864

ENH: Deprecate non-keyword arguments for interpolate

99ffd77

MarcoGorelli requested changes May 28, 2021

View reviewed changes

Anupam-USP added 2 commits May 28, 2021 20:08

ENH: Deprecate non-keyword arguments for interpolate

9f1363f

ENH: Deprecate non-keyword arguments for interpolate

4990a15

MarcoGorelli requested changes May 28, 2021

View reviewed changes

ENH: Deprecate non-keyword arguments for interpolate

afa41d0

MarcoGorelli requested changes May 28, 2021

View reviewed changes

Anupam-USP added 2 commits May 29, 2021 10:38

ENH: Deprecate non-keyword arguments for interpolate

b99fa40

Merge remote-tracking branch 'upstream/master' into Deprecation-for-i…

3d8eff7

…nterpolate

MarcoGorelli requested changes May 29, 2021

View reviewed changes

MarcoGorelli requested changes May 30, 2021

View reviewed changes

ENH: Deprecate non-keyword arguments for interpolate

f6076d2

Anupam-USP requested a review from MarcoGorelli May 30, 2021 12:50

MarcoGorelli requested changes May 30, 2021

View reviewed changes

jreback added Deprecate Functionality to remove in pandas Resample resample method labels May 31, 2021

ENH: Deprecate non-keyword arguments for interpolate

b47ecc2

Merge remote-tracking branch 'upstream/master' into Deprecation-for-i…

c27e606

…nterpolate

MarcoGorelli requested changes Jun 2, 2021

View reviewed changes

ENH: Deprecate non-keyword arguments for interpolate

2954950

MarcoGorelli reviewed Jun 4, 2021

View reviewed changes

simonjayhawkins mentioned this pull request Jun 4, 2021

Deprecate non-keyword arguments for methods with inplace #41485

Closed

31 tasks

Merge branch 'pandas-dev:master' into Deprecation-for-interpolate

3497b5a

simonjayhawkins changed the title ~~ENH: Deprecate non-keyword arguments for interpolate~~ ENH: Deprecate non-keyword arguments for Resampler.interpolate Jun 4, 2021

simonjayhawkins added this to the 1.3 milestone Jun 4, 2021

MarcoGorelli reviewed Jun 4, 2021

View reviewed changes

pandas/tests/resample/test_deprecated.py Outdated Show resolved Hide resolved

Update pandas/tests/resample/test_deprecated.py

7ff25da

MarcoGorelli merged commit 3f67dc3 into pandas-dev:master Jun 5, 2021

		with tm.assert_produces_warning(FutureWarning, match=msg):
		result = s.resample("3s").interpolate("linear")

ENH: Deprecate non-keyword arguments for Resampler.interpolate #41699

ENH: Deprecate non-keyword arguments for Resampler.interpolate #41699

Conversation

Anupam-USP commented May 28, 2021 • edited Loading

pep8speaks commented May 28, 2021 • edited Loading

Comment last updated at 2021-06-04 16:15:25 UTC

MarcoGorelli left a comment

Choose a reason for hiding this comment

Anupam-USP commented May 28, 2021

Choose a reason for hiding this comment

MarcoGorelli commented May 28, 2021

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anupam-USP commented May 29, 2021

Choose a reason for hiding this comment

Anupam-USP May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented May 31, 2021

Anupam-USP commented Jun 2, 2021

MarcoGorelli commented Jun 2, 2021

Anupam-USP commented Jun 2, 2021 • edited Loading

MarcoGorelli commented Jun 2, 2021 • edited Loading

Anupam-USP commented Jun 2, 2021

MarcoGorelli commented Jun 2, 2021

Anupam-USP commented Jun 2, 2021

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli commented Jun 5, 2021

Anupam-USP commented May 28, 2021 •

edited

Loading

pep8speaks commented May 28, 2021 •

edited

Loading

Anupam-USP May 29, 2021 •

edited

Loading

Anupam-USP commented Jun 2, 2021 •

edited

Loading

MarcoGorelli commented Jun 2, 2021 •

edited

Loading