DEPR: Remove unnecessary kwargs in groupby ops #50483

ramvikrams · 2022-12-29T06:21:25Z

closes DEPR: Unnecessary kwargs in groupby ops #50407
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

datapythonista · 2022-12-29T08:51:16Z

pandas/core/groupby/groupby.py

        """
        Cumulative sum for each group.

        Returns
        -------
        Series or DataFrame
        """
-        nv.validate_groupby_func("cumsum", args, kwargs, ["numeric_only", "skipna"])


The CI is failing because of this. You can't simply delete the values, you should provide an empty tuple and an empty dictionary instead.

Yes will do

i am getting this error Error: The runner has received a shutdown signal...' in GHA. GH 45651 , could you help on this.

datapythonista

We should add an entry in the whatsnew notes for this change.

ramvikrams · 2022-12-29T08:54:20Z

We should add an entry in the whatsnew notes for this change.

Will do

rhshadrach

Thanks for the PR! We should only be deprecating at this point. The arguments can only be removed after a deprecation period. The removal won't happen until we begin working on pandas 3.0.

ramvikrams · 2022-12-30T02:59:11Z

Thanks for the PR! We should only be deprecating at this point. The arguments can only be removed after a deprecation period. The removal won't happen until we begin working on pandas 3.0.

Thanks I will do the changes

ramvikrams · 2022-12-30T04:35:40Z

Thanks for the PR! We should only be deprecating at this point. The arguments can only be removed after a deprecation period. The removal won't happen until we begin working on pandas 3.0.

I looked over the net but could not find out how to deprecate this, can you please explain. I thought that we gad to remove the arguments when I opened the PR.

datapythonista · 2022-12-30T04:53:12Z

The idea is that in the next version we start showing warnings to the users in the next version, to let them know if they are passing the arguments that they will be removed, and some versions later we will actually remove them, once they had the time to update their code.

I'd personally still fix this PR with the removals first. The parameters are not veing used, but I wonder if there was any reason we are misssing to add them in the first place. So, it'll be good to see that nothing in our test suite breaks when removing them, before we apply the deprecation warnings.

For the warnings, check at closed PRs with the Deprecate label, and find one about arguments. We have a decorator to do it, but I'm in my phone and I fon't remember the exact name, something like deprecate_arg probably.

ramvikrams · 2022-12-30T07:03:00Z

thanks i'll look for closed pr with deprecate_arg

rhshadrach · 2022-12-30T15:10:00Z

@datapythonista - these arguments are to support passing groupby objects to NumPy; #47836 and #48277

ramvikrams · 2022-12-31T06:45:26Z

@datapythonista - these arguments are to support passing groupby objects to NumPy; #47836 and #48277

so should we add this maybe_warn_args_and_kwargs in all the methods, as the deprecation message.

datapythonista · 2022-12-31T07:52:42Z

Thanks @rhshadrach for the info, I wasn't aware of that, good to know.

@ramvikrams deprecate_nonkeyword_arguments is the decorator we need I think.

ramvikrams · 2022-12-31T11:18:41Z

Thanks @rhshadrach for the info, I wasn't aware of that, good to know.

@ramvikrams deprecate_nonkeyword_arguments is the decorator we need I think.

Yeah I'll push a commit in 30 mins

ramvikrams · 2022-12-31T16:22:23Z

I think it's done

rhshadrach

I think the changes look good except I'm not seeing cummin / cummax and this needs tests.

For tests, you want to invoke the function and ensure the appropriate warning is raised. Use with tm.assert_produces_warning(FutureWarning, match="..."): with the appropriate warning message filled in.

rhshadrach · 2023-01-02T12:38:56Z

doc/source/whatsnew/v2.0.0.rst

@@ -735,6 +735,7 @@ Removal of prior version deprecations/changes
 - Removed the deprecated argument ``line_terminator`` from :meth:`DataFrame.to_csv` (:issue:`45302`)
 - Removed the deprecated argument ``label`` from :func:`lreshape` (:issue:`30219`)
 - Arguments after ``expr`` in :meth:`DataFrame.eval` and :meth:`DataFrame.query` are keyword-only (:issue:`47587`)
+- Deprecate argument ``**kwargs`` from :meth:`cummax`, :meth:`cummin`, :meth:`cumsum`, :meth:`cumprod`, :meth:`take` and :meth:`skew` (:issue:`50483`)


This is currently in the section of deprecations that we are enforcing for 2.0. Instead, it should be in the new deprecations that are introduced in 2.0. That's one section up from this.

Can you add "unused" before "argument" to indicate to users we're not removing any functionality.

yes surely i'll do the changes

rhshadrach · 2023-01-09T03:27:10Z

pandas/tests/util/test_deprecate_nonkeyword_arguments.py

@@ -155,3 +165,17 @@ def test_class():
    )
    with tm.assert_produces_warning(FutureWarning, match=msg):
        Foo().baz("qux", "quox")
+
+
+def test_deprecated_keywords():


This file is for testing the decorator itself, these tests should go in pandas/tests/groupby/test_function.

rhshadrach · 2023-01-09T03:31:44Z

pandas/tests/util/test_deprecate_nonkeyword_arguments.py

+        "and kwargs will be deprecated for these functions"
+    )
+    with tm.assert_produces_warning(FutureWarning, match=msg):
+        assert cummax


assert [statement] evaluates the truthiness of the statement. For example, [] will evaluate to False (and hence raise if used in an assert) whereas [1] will evaluate to True. Functions are always truthy - they evaluate to True.

Instead, this needs to call the function itself, e.g.

df = DataFrame({'a': [1, 1, 2], 'b': [3, 4, 5]}) gb = df.groupby('a') gb.cummax(kwargs=1)

Oh thanks I'll do the changes

i did the changes but still the tests are failing, don't know why

datapythonista

Added some comments

datapythonista · 2023-01-10T04:10:25Z

pandas/tests/groupby/test_function.py

+        assert gb.cummax(kwargs=1)
+        assert gb.cummin(kwargs=1)
+        assert gb.cumsum(args=1, kwargs=1)
+        assert gb.cumprod(args=1, kwargs=1)
+        assert gb.skew(kwargs=1)
+        assert gb.take(kwargs=1)


assert is used to assert that a certain condition is true. Do you want to assert something here, or just call these functions?

Also, what will happen with this test if cummax produces the warning, but the other functions don't? Seems like the test will pass, even if the changes don't do what we want them to do. You probably want to use pytest parametrization, with the function name as a parameter, then you can retrieve the function with getattr(gb, func_name).

You probably want to use pytest parametrization, with the function name as a parameter, then you can retrieve the function with getattr(gb, func_name).

i'll try this way

datapythonista · 2023-01-10T04:16:44Z

pandas/core/groupby/generic.py

@@ -893,6 +894,9 @@ def fillna(
        return result

    @doc(Series.take.__doc__)
+    @deprecate_nonkeyword_arguments(
+        version="3.0", allowed_args=["self", "axis", "indices"]
+    )


Having another look, looks like deprecate_nonkeyword_arguments doesn't deprecate the argument, but makes it usable only via a keyword argument (e.g. foo(1) would produce the warning, but foo(val=1) wouldn't). If that's the case, doesn't seem like we've got a decorator to simply deprecate an argument. In that case, you'll have to produce it manually.

datapythonista · 2023-01-10T04:19:03Z

pandas/tests/groupby/test_function.py

+    with tm.assert_produces_warning(FutureWarning, match=msg):
+        df = DataFrame({"a": [1, 1, 2], "b": [3, 4, 5]})
+        gb = df.groupby("a")
+        assert gb.cummax(kwargs=1)


I think using kwargs as the sample keyword argument is a bit misleading. Something like gb.cummax(incorrect_argument=)` would probably be clearer.

datapythonista · 2023-01-10T04:21:29Z

pandas/tests/groupby/test_function.py

+        gb = df.groupby("a")
+        assert gb.cummax(kwargs=1)
+        assert gb.cummin(kwargs=1)
+        assert gb.cumsum(args=1, kwargs=1)


I think you've got this wrong. This is not testing args at all, to test if the *args deprecation is working you need to use gb.cumsum(1, incorrect_argument=1). And I'd recommend splitting this into two separate tests, otherwise you will only test that one of them works, not that both work.

I'd recommend that you make sure you understand well what the * and ** do in a function signature like def foo(my_arg, *args, **kwargs):

github-actions · 2023-02-10T04:01:11Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

datapythonista · 2023-02-11T20:46:41Z

@ramvikrams do you want to keep working on this PR?

ramvikrams · 2023-02-11T21:00:24Z

@ramvikrams do you want to keep working on this PR?

i think it's better I close it

ramvikrams added 2 commits December 29, 2022 11:20

Depr: unnecessary kwargs in groupby ops

7ebf822

update

88ab270

datapythonista reviewed Dec 29, 2022

View reviewed changes

datapythonista added Groupby Deprecate Functionality to remove in pandas labels Dec 29, 2022

datapythonista changed the title ~~gh-50407 Depr: unnecessary kwargs in groupby ops~~ DEPR: Remove unnecessary kwargs in groupby ops Dec 29, 2022

datapythonista reviewed Dec 29, 2022

View reviewed changes

datapythonista requested a review from rhshadrach December 29, 2022 08:53

added the whatsnew entey and modified the code

aff453d

rhshadrach requested changes Dec 29, 2022

View reviewed changes

ramvikrams added 5 commits December 31, 2022 19:10

added the decorator above the functions

4f6548b

update

2a680be

Update groupby.py

6569006

Update generic.py

3a150f6

Update v2.0.0.rst

519d9da

rhshadrach requested changes Jan 2, 2023

View reviewed changes

ramvikrams added 4 commits January 3, 2023 12:55

added tests

20c8606

Update v2.0.0.rst

b026a74

Update test_deprecate_nonkeyword_arguments.py

dfed625

Update test_deprecate_nonkeyword_arguments.py

1676d19

ramvikrams added 2 commits January 3, 2023 14:05

Update test_deprecate_nonkeyword_arguments.py

b3ca2f2

Update test_deprecate_nonkeyword_arguments.py

90fb8dc

datapythonista mentioned this pull request Jan 5, 2023

DEPR: List of deprecations to be removed in 3.0 #50578

Open

rhshadrach requested changes Jan 9, 2023

View reviewed changes

ramvikrams added 2 commits January 9, 2023 12:14

corrected the tests

fe8f551

Update test_deprecate_nonkeyword_arguments.py

e45c8ff

datapythonista requested changes Jan 10, 2023

View reviewed changes

github-actions bot added the Stale label Feb 10, 2023

ramvikrams closed this Feb 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: Remove unnecessary kwargs in groupby ops #50483

DEPR: Remove unnecessary kwargs in groupby ops #50483

ramvikrams commented Dec 29, 2022 •

edited

Loading

datapythonista Dec 29, 2022

ramvikrams Dec 29, 2022

ramvikrams Dec 29, 2022

datapythonista left a comment

ramvikrams commented Dec 29, 2022

rhshadrach left a comment

ramvikrams commented Dec 30, 2022

ramvikrams commented Dec 30, 2022

datapythonista commented Dec 30, 2022

ramvikrams commented Dec 30, 2022

rhshadrach commented Dec 30, 2022

ramvikrams commented Dec 31, 2022

datapythonista commented Dec 31, 2022

ramvikrams commented Dec 31, 2022

ramvikrams commented Dec 31, 2022

rhshadrach left a comment •

edited

Loading

rhshadrach Jan 2, 2023

ramvikrams Jan 2, 2023

rhshadrach Jan 9, 2023

rhshadrach Jan 9, 2023

ramvikrams Jan 9, 2023 •

edited

Loading

ramvikrams Jan 9, 2023

datapythonista left a comment

datapythonista Jan 10, 2023

ramvikrams Jan 10, 2023

datapythonista Jan 10, 2023

datapythonista Jan 10, 2023

datapythonista Jan 10, 2023

github-actions bot commented Feb 10, 2023

datapythonista commented Feb 11, 2023

ramvikrams commented Feb 11, 2023

DEPR: Remove unnecessary kwargs in groupby ops #50483

DEPR: Remove unnecessary kwargs in groupby ops #50483

Conversation

ramvikrams commented Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

ramvikrams commented Dec 29, 2022

rhshadrach left a comment

Choose a reason for hiding this comment

ramvikrams commented Dec 30, 2022

ramvikrams commented Dec 30, 2022

datapythonista commented Dec 30, 2022

ramvikrams commented Dec 30, 2022

rhshadrach commented Dec 30, 2022

ramvikrams commented Dec 31, 2022

datapythonista commented Dec 31, 2022

ramvikrams commented Dec 31, 2022

ramvikrams commented Dec 31, 2022

rhshadrach left a comment • 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

ramvikrams Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2023

datapythonista commented Feb 11, 2023

ramvikrams commented Feb 11, 2023

ramvikrams commented Dec 29, 2022 •

edited

Loading

rhshadrach left a comment •

edited

Loading

ramvikrams Jan 9, 2023 •

edited

Loading