BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True #49534

thlautenschlaeger · 2022-11-04T20:22:55Z

closes BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True #49352
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.

thlautenschlaeger · 2022-11-11T17:01:33Z

@rhshadrach can you please have a look at the PR since v1.5.2 will be released.

jreback · 2022-11-11T17:03:24Z

this doesn't have any tests

thlautenschlaeger · 2022-11-11T17:39:26Z

I wasn't sure if they are covered by default. Will add them. Thanks for your response.

thlautenschlaeger · 2022-11-11T17:53:58Z

To check in advance, is the appropriate location to add the test in the script below?:

pandas/tests/apply/test_frame_apply.py

rhshadrach · 2022-11-11T19:15:09Z

To check in advance, is the appropriate location to add the test in the script below?:

pandas/tests/apply/test_frame_apply.py

Yes - that's correct.

rhshadrach

Thanks for the PR!

For the overall the behavior here, I didn't realize commenting in the issue that this approach would be something of a hidden feature. That is, numeric_only isn't in the arguments of the function, but we're essentially implementing it as if it were.

I think this would be (at least more) okay if we were only exercising numeric_only when it came to string arguments. E.g. ['sum', lambda x, numeric_only: x.sum(), 'mean']` would do the computation on the numeric portion of the DataFrame for 'sum' and 'mean', but would attempt to pass every Series that makes up the frame to the lambda. This is admittedly not great - I think the only proper resolution is to use DataFrame methods in agg for list/dict like arguments, but that can't happen until 2.0.

cc @mroeschke for any thoughts.

pandas/core/apply.py

mroeschke · 2022-11-14T19:55:04Z

If it were better documented that passing the string alias of the DataFrame methods accepts the associated keyword arguments then I supposed it makes sense to support this, especially if the user passes multiple strings aliases into agg.

rhshadrach · 2022-11-14T22:26:36Z

Thanks @mroeschke - @thlautenschlaeger can you add a description of whats happening here to the notes section of agg and apply.

thlautenschlaeger · 2022-11-16T00:20:46Z

Thanks @mroeschke - @thlautenschlaeger can you add a description of whats happening here to the notes section of agg and apply.

@rhshadrach Added the description to that section. Also added an example to the doc for agg. Hope everything is alright now.

mroeschke · 2022-11-16T22:33:49Z

pandas/core/frame.py

+    ...                   columns=['A', 'B', 'C'])
+
+    Works equivalently as above. Add argument `numeric_only=True` to avoid
+    exceptions or warnings.


Nit: The warning will go away soon (in 2.0) so I am not sure it's worth including here

I believe you're just talking about the or warnings bit, is that right @mroeschke? Agreed - I think the more general to aggregate only numeric columns. would be sufficient - just like the apply docstring.

mroeschke · 2022-11-16T22:35:12Z

doc/source/whatsnew/v1.5.2.rst

@@ -27,7 +27,7 @@ Fixed regressions
 Bug fixes
 ~~~~~~~~~
 - Bug in the Copy-on-Write implementation losing track of views in certain chained indexing cases (:issue:`48996`)
-
+- Bug in inconsistent DataFrame.agg behavior when passing as kwargs ``numeric_only=True`` (:issue:`49352`


Suggested change

- Bug in inconsistent DataFrame.agg behavior when passing as kwargs ``numeric_only=True`` (:issue:`49352`

- Bug in inconsistent DataFrame.agg behavior when passing as kwargs ``numeric_only=True`` with :class:`DataFrame` methods or their associated string alias as functions (:issue:`49352`)

I don't we should be mentioning DataFrame methods here (#49352 (comment) and #49352 (comment)). +1 on the "string alias" bit though.

mroeschke

@rhshadrach I'm assuming the other kwargs like skipna and min_periods have the same issue here in agg, but those don't need to be addressed in 1.5.x since those don't affect the FutureWarning?

rhshadrach

This is looking good - a few requests below. Also, when mentioning the numeric_only in the docs, can you state it applies only to string arguments. That it works for e.g. DataFrame.mean is part of the standard behavior of agg (to pass any keywords through to the provided function), so I don't think that needs to be particularly called out.

rhshadrach · 2022-11-16T22:39:33Z

pandas/core/apply.py

@@ -284,6 +284,14 @@ def transform_str_or_callable(self, func) -> DataFrame | Series:
        except Exception:
            return func(obj, *args, **kwargs)

+    def _filter_numeric_only(self) -> list[Any]:
+        if "numeric_only" in self.kwargs and self.kwargs["numeric_only"] is True:


Why is True here? I haven't checked all, but I believe DataFrame methods use "truthy" (i.e. and self.kwargs["numeric_only"]):

df = pd.DataFrame({'a': [1, 1, 2], 'b': list('xyz')}) print(df.sum(numeric_only="string")) # a 4 # dtype: int64

I don't believe there is any need for the is True here. This would give surprising results if the user passes e.g. np.bool_(True).

Also, can you use kwargs.get("numeric_only", False)

pandas/core/apply.py

rhshadrach · 2022-11-16T22:46:53Z

pandas/core/frame.py

+    ...                   columns=['A', 'B', 'C'])
+
+    Works equivalently as above. Add argument `numeric_only=True` to avoid
+    exceptions or warnings.


I believe you're just talking about the or warnings bit, is that right @mroeschke? Agreed - I think the more general to aggregate only numeric columns. would be sufficient - just like the apply docstring.

pandas/tests/apply/test_frame_apply.py

rhshadrach · 2022-11-16T22:58:17Z

doc/source/whatsnew/v1.5.2.rst

@@ -27,7 +27,7 @@ Fixed regressions
 Bug fixes
 ~~~~~~~~~
 - Bug in the Copy-on-Write implementation losing track of views in certain chained indexing cases (:issue:`48996`)
-
+- Bug in inconsistent DataFrame.agg behavior when passing as kwargs ``numeric_only=True`` (:issue:`49352`


I don't we should be mentioning DataFrame methods here (#49352 (comment) and #49352 (comment)). +1 on the "string alias" bit though.

pandas/core/frame.py

rhshadrach · 2022-11-16T23:02:02Z

@rhshadrach I'm assuming the other kwargs like skipna and min_periods have the same issue here in agg, but those don't need to be addressed in 1.5.x since those don't affect the FutureWarning?

Correct. While I do think things like skipna should work, they are not part of the regression due to the numeric_only changes.

pandas/core/apply.py

thlautenschlaeger · 2022-11-17T16:52:30Z

Thanks for your reviews @mroeschke @rhshadrach
I will go through your comments and integrate the change requests.

rhshadrach

I think we need to only filter the DataFrame due to numeric_only on string arguments in the list-like. Otherwise, we will be creating an inconsistency between list and callable arguments. For example, with f a callable user-defined function

df.agg(f, numeric_only=True)

will pass numeric_only to the function f whereas

df.agg([f], numeric_only=True)

will filter the DataFrame prior to calling f.

pandas/core/apply.py

pandas/core/frame.py

pandas/core/shared_docs.py

rhshadrach · 2022-12-01T03:10:49Z

pandas/core/apply.py

@@ -284,6 +284,14 @@ def transform_str_or_callable(self, func) -> DataFrame | Series:
        except Exception:
            return func(obj, *args, **kwargs)

+    def _filter_numeric_only(self) -> list[Any]:
+        if "numeric_only" in self.kwargs and self.kwargs["numeric_only"] is True:


I don't believe there is any need for the is True here. This would give surprising results if the user passes e.g. np.bool_(True).

Also, can you use kwargs.get("numeric_only", False)

rhshadrach · 2022-12-01T03:12:35Z

pandas/core/frame.py

+    ...                    ['c', 3, 6]],
+    ...                   columns=['A', 'B', 'C'])
+
+    Works equivalently as above. Add argument `numeric_only=True` to


What does "Works equivalently as above." mean here?

rhshadrach · 2022-12-01T03:18:39Z

pandas/core/apply.py

@@ -294,7 +299,7 @@ def agg_list_like(self) -> DataFrame | Series:
        """
        from pandas.core.reshape.concat import concat

-        obj = self.obj
+        obj = self._maybe_filter_numeric_only()


This will cause different behaviors for list and non-list arguments that contain a callable. E.g.

df.agg(lambda x: x.sum(), numeric_only=True)) df.agg([lambda x: x.sum()], numeric_only=True)

Currently in this PR, I believe the first line will pass numeric_only to the supplied UDF (and this fails as the UDF doesn't take a numeric_only argument). The second line will subset the DataFrame to only numeric columns and succeed. These should have the same behavior with regards to how the numeric_only argument is treated.

I suggest only using the DataFrame filtered to numeric_only when a given argument in the list is a string; otherwise to use the full DataFrame and pass the numeric_only kwarg to the supplied UDF.

rhshadrach · 2022-12-01T03:21:43Z

pandas/tests/apply/test_frame_apply.py

@@ -858,6 +858,22 @@ def test_with_dictlike_columns_with_infer():
    tm.assert_frame_equal(result, expected)


+def test_with_dictlike_functions():


We're no longer changing dictlike behavior, I think this can be removed. It should fail, but that is another (unrelated) issue.

github-actions · 2023-01-01T00:06:14Z

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.

rhshadrach · 2023-01-02T02:23:00Z

@thlautenschlaeger - friendly ping. Are you interested in continuing?

mroeschke · 2023-01-23T19:49:57Z

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

thlautenschlaeger changed the title ~~add func list handling for numeric_only~~ BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True Nov 4, 2022

mroeschke requested a review from rhshadrach November 7, 2022 19:03

mroeschke added Apply Apply, Aggregate, Transform, Map Warnings Warnings that appear or should be added to pandas and removed Warnings Warnings that appear or should be added to pandas labels Nov 7, 2022

rhshadrach reviewed Nov 11, 2022

View reviewed changes

pandas/core/apply.py Outdated Show resolved Hide resolved

thlautenschlaeger force-pushed the fix_numeric_list_func branch from e35fa47 to 749a9eb Compare November 15, 2022 23:40

mroeschke reviewed Nov 16, 2022

View reviewed changes

rhshadrach requested changes Nov 16, 2022

View reviewed changes

rhshadrach reviewed Nov 16, 2022

View reviewed changes

pandas/core/apply.py Outdated Show resolved Hide resolved

thlautenschlaeger force-pushed the fix_numeric_list_func branch from 32d50db to 749a9eb Compare November 26, 2022 01:59

Thomas Lautenschlaeger added 7 commits November 26, 2022 03:20

add func list handling for numeric_only

200c9fb

added tests

8505c49

added issue id to tests

887c3c5

fixed test

8fab5a8

fixed typing issue

297c53d

added notes for documentation

ca3a7c1

fixed docstring

a4e7c1a

Thomas Lautenschlaeger added 2 commits November 26, 2022 03:20

applied patch

435347f

applied patch

bf86a8d

thlautenschlaeger force-pushed the fix_numeric_list_func branch from 10ffdde to bf86a8d Compare November 26, 2022 02:20

added change to whats new

3d55579

rhshadrach requested changes Nov 26, 2022

View reviewed changes

pandas/core/apply.py Outdated Show resolved Hide resolved

pandas/core/apply.py Outdated Show resolved Hide resolved

pandas/core/frame.py Outdated Show resolved Hide resolved

pandas/core/frame.py Show resolved Hide resolved

pandas/core/shared_docs.py Show resolved Hide resolved

Thomas Lautenschlaeger and others added 3 commits November 28, 2022 23:11

resolved threads

2a2f0c0

Merge branch 'main' into fix_numeric_list_func

de584bf

fix doc

61a4b50

thlautenschlaeger requested a review from rhshadrach November 29, 2022 19:04

rhshadrach requested changes Dec 1, 2022

View reviewed changes

github-actions bot added the Stale label Jan 1, 2023

rhshadrach mentioned this pull request Jan 3, 2023

API: GroupBy.agg() numeric_only deprecation with custom function #50538

Open

mroeschke closed this Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True #49534

BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True #49534

thlautenschlaeger commented Nov 4, 2022 •

edited

Loading

thlautenschlaeger commented Nov 11, 2022

jreback commented Nov 11, 2022

thlautenschlaeger commented Nov 11, 2022

thlautenschlaeger commented Nov 11, 2022

rhshadrach commented Nov 11, 2022

rhshadrach left a comment •

edited

Loading

mroeschke commented Nov 14, 2022

rhshadrach commented Nov 14, 2022

thlautenschlaeger commented Nov 16, 2022 •

edited

Loading

mroeschke Nov 16, 2022

rhshadrach Nov 16, 2022

mroeschke Nov 16, 2022

rhshadrach Nov 16, 2022

mroeschke left a comment

rhshadrach left a comment

rhshadrach Nov 16, 2022

rhshadrach Dec 1, 2022

rhshadrach Nov 16, 2022

rhshadrach Nov 16, 2022

rhshadrach commented Nov 16, 2022

thlautenschlaeger commented Nov 17, 2022

rhshadrach left a comment

rhshadrach Dec 1, 2022

rhshadrach Dec 1, 2022

rhshadrach Dec 1, 2022

rhshadrach Dec 1, 2022

github-actions bot commented Jan 1, 2023

rhshadrach commented Jan 2, 2023

mroeschke commented Jan 23, 2023

	- Bug in inconsistent DataFrame.agg behavior when passing as kwargs ``numeric_only=True`` (:issue:`49352`
	- Bug in inconsistent DataFrame.agg behavior when passing as kwargs ``numeric_only=True`` with :class:`DataFrame` methods or their associated string alias as functions (:issue:`49352`)

		@@ -858,6 +858,22 @@ def test_with_dictlike_columns_with_infer():
		tm.assert_frame_equal(result, expected)


		def test_with_dictlike_functions():

BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True #49534

BUG: inconsistent DataFrame.agg behavoir when passing as kwargs numeric_only=True #49534

Conversation

thlautenschlaeger commented Nov 4, 2022 • edited Loading

thlautenschlaeger commented Nov 11, 2022

jreback commented Nov 11, 2022

thlautenschlaeger commented Nov 11, 2022

thlautenschlaeger commented Nov 11, 2022

rhshadrach commented Nov 11, 2022

rhshadrach left a comment • edited Loading

Choose a reason for hiding this comment

mroeschke commented Nov 14, 2022

rhshadrach commented Nov 14, 2022

thlautenschlaeger commented Nov 16, 2022 • 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

mroeschke left a comment

Choose a reason for hiding this comment

rhshadrach 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

rhshadrach commented Nov 16, 2022

thlautenschlaeger commented Nov 17, 2022

rhshadrach 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

github-actions bot commented Jan 1, 2023

rhshadrach commented Jan 2, 2023

mroeschke commented Jan 23, 2023

thlautenschlaeger commented Nov 4, 2022 •

edited

Loading

rhshadrach left a comment •

edited

Loading

thlautenschlaeger commented Nov 16, 2022 •

edited

Loading