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

Updating _resolve_numeric_only function of GroupBy #43154

Merged
merged 38 commits into from
Sep 9, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
6b0f5e7
Updating _resolve_numeric_only function of GroupBy
kurchi1205 Aug 21, 2021
116534f
Update groupby.py
kurchi1205 Aug 21, 2021
7b5ecb4
Update test_aggregate.py
kurchi1205 Aug 21, 2021
7faf1fc
Update groupby.py
kurchi1205 Aug 21, 2021
d773e7a
Update groupby.py
kurchi1205 Aug 21, 2021
5b4e799
Update test_aggregate.py
kurchi1205 Aug 21, 2021
3d2e78e
Update groupby.py
kurchi1205 Aug 21, 2021
5836f91
Update groupby.py
kurchi1205 Aug 21, 2021
1eb0e25
Update groupby.py
kurchi1205 Aug 21, 2021
a0391e6
Update test_aggregate.py
kurchi1205 Aug 21, 2021
2a1835a
Update test_aggregate.py
kurchi1205 Aug 21, 2021
66ecb96
Update test_aggregate.py
kurchi1205 Aug 21, 2021
c80fa9e
Update groupby.py
kurchi1205 Aug 22, 2021
95b6533
Update groupby.py
kurchi1205 Aug 22, 2021
fa8a8ca
Update test_aggregate.py
kurchi1205 Aug 22, 2021
e3f6767
Update test_aggregate.py
kurchi1205 Aug 22, 2021
ed668e6
Update groupby.py
kurchi1205 Aug 22, 2021
f30855a
Update groupby.py
kurchi1205 Aug 22, 2021
65261b4
Update groupby.py
kurchi1205 Aug 22, 2021
2477aa3
Update groupby.py
kurchi1205 Aug 22, 2021
da24f29
Update groupby.py
kurchi1205 Aug 22, 2021
4075353
Update groupby.py
kurchi1205 Aug 24, 2021
463a7c9
Update test_aggregate.py
kurchi1205 Aug 24, 2021
dd562c5
Update frame.py
kurchi1205 Aug 24, 2021
f400174
Making the necessary to _resolve_numeric_only and adding testcase
kurchi1205 Aug 29, 2021
56f28d7
Checking for empty DataFrame
kurchi1205 Aug 29, 2021
4fd254b
Commiting
kurchi1205 Aug 30, 2021
a3b09e6
Adding test cases
kurchi1205 Aug 30, 2021
f7e2d59
No changes
kurchi1205 Aug 30, 2021
270549a
Changes in tests and warning
kurchi1205 Sep 1, 2021
17808bf
Solving the errors
kurchi1205 Sep 1, 2021
08b2429
Solving the errors
kurchi1205 Sep 1, 2021
e141123
Merge remote-tracking branch 'upstream/master' into pr43154
Dr-Irv Sep 9, 2021
45f54d6
whatsnew 1.3.3, move tests, restore mypy
Dr-Irv Sep 9, 2021
0012423
add back blank line
Dr-Irv Sep 9, 2021
a9a63e7
Merge branch 'master' into issue_43108
jreback Sep 9, 2021
d46091e
add FutureWarning. Avoid Int64Index
Dr-Irv Sep 9, 2021
a39e5ce
Merge remote-tracking branch 'upstream/master' into issue_43108
Dr-Irv Sep 9, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pandas/core/groupby/groupby.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class providing the base-class of operations.

from contextlib import contextmanager
import datetime
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

this is already imported below

from functools import (
partial,
wraps,
Expand Down Expand Up @@ -1119,7 +1120,15 @@ def _resolve_numeric_only(self, numeric_only: bool | lib.NoDefault) -> bool:
# i.e. not explicitly passed by user
if self.obj.ndim == 2:
# i.e. DataFrameGroupBy
numeric_only = True
# Checking if the dataframe has non-numeric features
Copy link
Contributor

Choose a reason for hiding this comment

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

this is very complicated, see if you can do this in a simpler way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok , working on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated a simpler version

Copy link
Member

Choose a reason for hiding this comment

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

ill need to look at the linked issue more closely, but im very skeptical that this method is the right place for a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicitly numeric_only =False solves the error , so I looked into _resolve_numeric_only function.
It only checked for dataframe groupby object and set numeric_only to True , which forced the non_numeric columns out of the aggregation . That's why I included checks for multi datatype dataframe .

Copy link
Member

Choose a reason for hiding this comment

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

The point of numeric_only is to force non-numeric columns out of the aggregation. Changing the behavior here "fixed" the case you tested, but also changed a bunch of other behavior that broke a bunch of other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I get ur part, I will check for numeric columns, if there are none only then it should set numeric_only to false, that should solve the issue and not violate the documentation

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure you do.

numeric_only is an argument passed by the user, or defaulting to True if not explicitly passed. We should never be changing it based on what dtypes are actually present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why can't the default value be set checking the datatypes , why does it have to be True always .


non_num_cols = self.obj.select_dtypes(
exclude=[np.number, np.datetime64, np.timedelta64]).columns

if len(set(non_num_cols) - set(self.keys)) > 0:
numeric_only = False
else:
numeric_only = True
else:
numeric_only = False

Expand Down
12 changes: 12 additions & 0 deletions pandas/tests/groupby/aggregate/test_aggregate.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,19 @@ def test_groupby_aggregation_mixed_dtype():
g = df.groupby(["by1", "by2"])
result = g[["v1", "v2"]].mean()
tm.assert_frame_equal(result, expected)
expected2 = DataFrame(
Copy link
Contributor

Choose a reason for hiding this comment

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

make a new test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will work on that tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have developed a new test case .

{
"v1": [15, 7, 9, 3, 3, 5],
"v2": [165, 77, 99, 33, 33, 55],
"by2": [293, 194, 0, 'damp', 'dry', 'wetred']
},
index=Index([1, 2, 12, 'big', 'blue', 'red'],
dtype='object', name='by1'),
)

g = df.groupby(["by1"])
result = g.sum()
tm.assert_frame_equal(result, expected2)

def test_groupby_aggregation_multi_level_column():
# GH 29772
Expand Down