Updating _resolve_numeric_only function of GroupBy
State: closed

- [x] closes #43108 and #42395
   It  sets the numeric_only value based on the columns on which aggregate functions are applied
- [x] tests added / passed
   New Test is added to  `test_groupby_aggregation_mixed_dtype` function
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [x] whatsnew entry
   Checking the column DataTypes in `_resolve_numeric_only` function 

I am absolutely new to contributing on Open Source . So please guide me through it .


Labels:
name: Groupby
name: Nuisance Columns
description: Identifying/Dropping nuisance columns in reductions, groupby.add, DataFrame.apply

Comments:
Hello @kurchi1205! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







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

##### Comment last updated at 2021-09-09 16:40:13 UTC
Commented on #43108, I think this is correct as-is.
Changing the default is something we can do, but that is an API change, not a bugfix, and would require a deprecation cycle.
ok , so how to go about with that ? I don't know how that works ?
> ok , so how to go about with that ? I don't know how that works ?

First there needs to be consensus that we _want_ to change the behavior.  That discussion seems to be in #43108.  Then a PR that doesn't change the behavior, but does issue a warning that the behavior will change in a future version, e.g. #42738
ok So I will create a different pull request and issue a warning in groupby.py 
@kurchi1205 I've closed #43108 as a duplicate of #42395.

but okay to have tests for both issue reports. can you also add the code sample from #42395
ok I will add the test for issue #42395 
@simonjayhawkins  ok So I will create a different pull request and issue a warning in groupby.py 
and write the code for changing the behavior in the current pull request . 
I am sort of stuck in this deprecation cycle process 
> So I will create a different pull request and issue a warning in groupby.py

no need to open another PR, can keeping pushing to this one as the issue discussion progresses.

we don't deprecate behavior in a patch release so if this is the way forward, this PR targets 1.4

if we need to revert any behavior changes first, we would probably want to backport, i.e. target 1.3.3
ok thanks 
I have maintained the original behaviour and issued a warning in case all the columns are non numeric
> I have maintained the original behaviour and issued a warning in case all the columns are non numeric

If this is the agreed way forward, we need two PRs.

one restoring the old behavior targeting 1.3.3 and one adding the deprecation to target 1.4

can you add release notes and also we need to test for the deprecation warning
sorry for appearing to contradict the earlier comment, but at that point the suggestion was to issue a warning and keep the current behavior.

if done, a PR restoring 1.2 behavior should be done first. adding a deprecation warning should probably wait till that's agreed and merged. There is a risk of conflicts making the backport less straightforward if done in the opposite order.
Ok so right now , I will create a pull request that restores 1.2 behaviour ie. numeric_only is set to True  . After that once the next steps are decided  , I will add the deprecation warning in another pull request
> Ok so right now , I will create a pull request that restores 1.2 behaviour ie. numeric_only is set to True .

I'm not so sure that numeric_only is set to True, we need to identify when the user specifies `numeric_only` explicitly?

my understanding is that to fix the regression, we need to restore the default behavior when the Dataframe or Groupby object has only one column and that column is not numeric but also drop the column from the results when `numeric_only=True` is explicitly passed. That should probably not need any changes to existing tests and with the code samples from the two issues added and passing?

That PR (or this one if pushing changes here) would target 1.3.3 and need a release note in doc/source/whatsnew/v1.3.3.rst

That would effectively close the existing issue from the regression perspective but we would want to also confirm what the expected future behavior would be. We could open a new issue or keep that one open for further discussion.

cc @Dr-Irv @jbrockmendel @rhshadrach 
> my understanding is that to fix the regression, we need to restore the default behavior when the Dataframe or Groupby object has only one column and that column is not numeric but also drop the column from the results when `numeric_only=True` is explicitly passed.

It's not just for _one_ column.  If _all_ of the columns are non-numeric, then the reduction (e.g., `sum`) is applied and all columns are returned.   Example below is with pandas 1.2.5:

```python
>>> df = pd.DataFrame({"x": [1,0,1,1,0], "y": [pd.Timedelta(i, "days") for i in range(1,6)], "z": [pd.Timedelta(i*10, "days") for i in range(1,6)]})
>>> df
   x      y       z
0  1 1 days 10 days
1  0 2 days 20 days
2  1 3 days 30 days
3  1 4 days 40 days
4  0 5 days 50 days
>>> df.groupby("x").sum()
       y       z
x
0 7 days 70 days
1 8 days 80 days
```
Interestingly, if `numeric_only=True`, you get the same result, which one could argue is a bug
```python
>>> df.groupby("x").sum(numeric_only=True)
       y       z
x
0 7 days 70 days
1 8 days 80 days
```
> That would effectively close the existing issue from the regression perspective but we would want to also confirm what the expected future behavior would be. We could open a new issue or keep that one open for further discussion.

I agree with this.
```      
def _resolve_numeric_only(self, numeric_only: bool | lib.NoDefault) -> bool:
        """
        Determine subclass-specific default value for 'numeric_only'.

        For SeriesGroupBy we want the default to be False (to match Series behavior).
        For DataFrameGroupBy we want it to be True (for backwards-compat).

        Parameters
        ----------
        numeric_only : bool or lib.no_default

        Returns
        -------
        bool
        """
        # GH#41291
    
        if numeric_only is lib.no_default:
            # i.e. not explicitly passed by user
            if self.obj.ndim == 2:
                # i.e. DataFrameGroupBy
                numeric_cols = [x for x in self.obj.columns if is_numeric_dtype(self.obj[x]) ]
                num_cols_to_agg = len(set(numeric_cols) - set(self.grouper.names))
                if num_cols_to_agg > 0:
                    numeric_only = True
                else:
                    numeric_only = None
            else:
                numeric_only = None

        # error: Incompatible return value type (got "Union[bool, NoDefault]",
        # expected "bool")
        return numeric_only  # type: ignore[return-value]
```

For #43108 , if numeric_only is set to True explicitly then column v will be dropped otherwise , 'v' will be aggregated.
for #42395, if numeric_only is set to True explicitly then column value will be dropped otherwise,it will be aggregated .

for https://github.com/pandas-dev/pandas/pull/43154#issuecomment-905450396  ,  for default value of numeric_only , it will aggregate and explicitly for numeric_only=True , it will drop the columns 
I think this is the behaviour expected
> numeric_only = None
>             else:
>                 numeric_only = None

Should be `False`, not `None`.  If you're going to propose these changes, please make sure to create tests that test various combinations of numeric and non numeric columns, with one, two and three columns.
yeah , if this is agreed by everyone , i will definitely create tests in the pull request . 
I'm not wild about making this more complicated and re-changing the behavior.  The old behavior was a bug, and there is a clear solution (pass numeric_only=False) for users affected by the change.

That said, if I'm alone in this I won't press the point.  cc @jreback
> I'm not wild about making this more complicated and re-changing the behavior. The old behavior was a bug, and there is a clear solution (pass numeric_only=False) for users affected by the change.

The behavior that existed prior to 1.3 existed at least in version 0.24.2 (the oldest one I have in my machine).  That's at least 2.5 years.  And with the way 1.3 is now, there are people (e.g., a member of my staff) who depended on the old behavior whose code will now break.  The `numeric_only` flag in a `groupby.sum()` operation was documented first in version 1.1 (based on what I can see in the docs).  I can't even find a "whatsnew" entry for `numeric_only` and `groupby`.  

> I can't even find a "whatsnew" entry for numeric_only and groupby.

In the 1.3.0 doc, feel free to edit to include "numeric_only"
```
- Bug in :class:`DataFrameGroupBy` aggregations incorrectly failing to drop columns with invalid dtypes for that aggregation when there are no valid columns (:issue:`41291`)
```
So shall we keep the current behaviour and mention numeric_only=False whenever necessary , or make change to the default value?

@simonjayhawkins @jbrockmendel @jreback @Dr-Irv 
> So shall we keep the current behaviour and mention numeric_only=False whenever necessary , or make change to the default value?

I don't want to see **any** changes for now other than restoring 1.2 behavior for the case where the user does not explicitly pass `numeric_only`. i.e. no existing tests changes, no new warnings, no changes to keyword defaults, so that we can backport and close the regression issue.

@jbrockmendel needs to agree to this.

If it is decided not to fix the regression, then the future behavior still needs discussion. if that is to change the default to False, then the current behaviour (for the reported regressions) will change back to the 1.2 behaviour in a future release after a deprecation cycle.

I think changing the default behavior back and forth in a "short" time frame is not ideal. If the future is not to change the default to False, then @jbrockmendel comment https://github.com/pandas-dev/pandas/pull/43154#issuecomment-905811000 could potentially be justified.
> then @jbrockmendel comment [#43154 (comment)](https://github.com/pandas-dev/pandas/pull/43154#issuecomment-905811000) could potentially be justified.

it's a grey area. from https://pandas.pydata.org/pandas-docs/dev/development/policies.html#version-policy

> pandas will sometimes make behavior changing bug fixes, as part of minor or patch releases. Whether or not a change is a bug fix or an API-breaking change is a judgement call. We’ll do our best, and we invite you to participate in development discussion on the issue tracker or mailing list.

the policy also states...

> API breaking changes should only occur in major releases. These changes will be documented, with clear guidance on what is changing, why it’s changing, and how to migrate existing code to the new behavior.

> Whenever possible, a deprecation path will be provided rather than an outright breaking change.

So as this is possible, even though it is making this more complicated than necessary for now, the regression should probably be fixed. 
> I don't want to see **any** changes for now other than restoring 1.2 behavior for the case where the user does not explicitly pass `numeric_only`. i.e. no existing tests changes, no new warnings, no changes to keyword defaults, so that we can backport and close the regression issue.

@simonjayhawkins I'm in agreement with you.  But if we do as you say here, would a new test (as opposed to modifying an existing test) be added to check that we have restored the 1.2 behavior?
> @simonjayhawkins I'm in agreement with you. But if we do as you say here, would a new test (as opposed to modifying an existing test) be added to check that we have restored the 1.2 behavior?

sure. the code sample for the issue as a minimum but also happy to add others as you suggested in https://github.com/pandas-dev/pandas/pull/43154#issuecomment-905480047

IIUC no existing tests would need to be changed as the commit that caused the regression, #41706 did not change or add tests relevant to these cases, all the test changes in #41706 specified `numeric_only` explicitly and removed tests for the default case. (These probably need to be reinstated to be sure of maintaining the previous behavior)

(there was also a change to test_cython_agg_nothing_to_agg which i'm not sure about. It looks like a case that used to raise, now returns an empty DataFrame, not checked to see if that is a bugfix or an api change, i'm considering that one out of scope for now)

IIUC #41706 did fix cases for when the user passes `numeric_only`, and I have been quite happy to retain those changes. If this all gets too complex, then reverting that PR maybe a better option.
tbc. I've been using the term "test changes" and maybe that is not clear. As adding or removing a test, or part of a test is a change.

In the places i've used "test changes", I mean that the tested behavior does not change. I think the issue is with (now) untested behavior and that the tested behavior returns the correct results (i.e. fixes the cases for `numeric_only=True` specified explicitly) wheras I have been using the term default behaviour for when `numeric_only` is not specified.
> So as this is possible, even though it is making this more complicated than necessary for now, the regression should probably be fixed.

Sure.  So IIUC the idea is to change _resolve_numeric_only to look like:

```
def _resolve_numeric_only(self, numeric_only) -> bool:
        if numeric_only is lib.no_default:
            # i.e. not explicitly passed by user
            if self.obj.ndim == 2:
                # i.e. DataFrameGroupBy
                numeric_only = True
                obj = self._obj_with_exclusions   # or self.obj?  need to be careful here
                check = obj._get_numeric_data()
                if len(obj.columns) and not len(check.columns):
                    warnings.warn("... Explicitly pass numeric_only ...")
                    numeric_only=False
                     
            else:
                numeric_only = False
    
```