API/DEPR: Change default skipna behaviour + deprecate numeric_only in Categorical.min and max
State: closed

- closes #25303 and follow up https://github.com/pandas-dev/pandas/pull/27801#discussion_r313075771
- 1 test added
- passes `black pandas`
- passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- whatsnew entry


Labels:
name: API Design
name: Categorical
description: Categorical Data Type
name: Deprecate
description: Functionality to remove in pandas

Comments:
@jreback Anything else? Please tell me.
The default behavior will change when  `Categorical` contains NA values. In the present code, `numeric_only` is `None` by default. No matter NA value is involved, the min operation is carried over the entire categorical, i.e., the else clause.

https://github.com/pandas-dev/pandas/blob/ea60c1966bf7291829a1479512d7aa89d08bd6dd/pandas/core/arrays/categorical.py#L2236-L2240

The `Categorical.min` returns `nan` but `Categorical.max` doesn't if `nan` is contained. `-1` stands for `nan` which is usually the minimum in `Categorical._code`
```
In [22]: from pandas import Categorical

In [23]: cat = Categorical([np.nan, 1, 2, np.nan], ordered=True)

In [24]: cat.min()
Out[24]: nan

In [25]: cat.max()
Out[25]: 2
```
In this PR, the default behavior of `min` and `max` is to drop NA values in advance, i.e., `skipna=True`.
cat.min() should return 1 and cat.max() return 2.


OK, that behavior looks pretty buggy.  But I'm not sure if we should be just changing the default output of `.min()` or `.max()`.... @jorisvandenbossche do you have thoughts here?

Given that users will need to update their code anyway to use the new argument, I think that we should try to get the correct behavior when `skipna=True`, while preserving the buggy bheavior with `skipna=False`. How much of a hassle will that be?

I also think the error message can be improved.

```python
In [6]: c.max(numeric_only=False)
/Users/taugspurger/.virtualenvs/pandas-dev/bin/ipython:1: FutureWarning: the 'numeric_only' keyword is deprecated, use 'skipna' instead
  #!/Users/taugspurger/Envs/pandas-dev/bin/python
Out[6]: nan

```

Reading that, it seems like I just need to replace `numeric_only` with `skipna`.  But I also need to invert the value. The warning should indicate that.
@jorisvandenbossche Would you tell us your thought about Tom's suggestion? That is keeping the buggy behaviour when `skipna=False`, while having a desired behaviour when `skipna=True`.
@makbigc can you merge master.

@jorisvandenbossche can you respond to questions here: https://github.com/pandas-dev/pandas/pull/27929#issuecomment-524869043
Focusing on the default behaviour for a moment, so when no arguments are specified (and not how to handle the numeric_only keyword): **what default behaviour do we want?**

Currently, `Series.min` (and also Categorical.min) returns the "wrong" thing:

```
In [28]: cat = pd.Categorical([1, 2, np.nan], ordered=True) 

In [29]: pd.Series(cat).min()
Out[29]: nan
```

To be consistent with the rest of pandas, this result should be 1 instead of nan (since we have a default `skipna=True`).

I think we agree that we want that correct behaviour long term? 

Question is then how to get there:

- breaking change: start skipping NaNs by default in 1.0
- first introduce a warning that in the future this will skip NaNs by default. We could only raise this warning if we detect that there are actually NaNs present (so a case where the result would change). And in this case, the user could silence the warning by specifying explicitly `skipna=True`

Personally, I might have a slight preference to actually do a breaking change on this for 1.0 for the default behaviour (we still need to deprecate the numeric_only keyword when specified, that's a separate thing). But, it's certainly possible to do it with a deprecation (we will only need to change `skipna=True` into `skipna=None` to know when it was explicitly specified by the user).







@jorisvandenbossche Thanks for your detail reply.

If I take the first approach (i.e., breaking change), what I should do is:

1. Remove the deprecation message that the numeric_only is replaced with skipna
2. In whatsnew, state explicitly the previous behaviour and the changed behaviour

Anything else? Please tell me.
Let's wait a bit to see what others think about the default behaviour. 
But in any case, we want to keep the deprecation of `numeric_only` keyword (passing it should raise a warning)
I agree we should deprecate ``numeric_only``; I think we need to default ``skipna=None`` and then warn; changing this in the future.

Though a breaking change is simpler.
@TomAugspurger thoughts on https://github.com/pandas-dev/pandas/pull/27929#issuecomment-538886984 ?
That seems good. 

> On Oct 20, 2019, at 07:35, Joris Van den Bossche <notifications@github.com> wrote:
> 
> ﻿
> @TomAugspurger thoughts on #27929 (comment) ?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

Sorry, in the linked comment I ask several questions. So what is the "that seems good" exactly answering to?
> Sorry, in the linked comment I ask several questions. So what is the "that seems good" exactly answering to?

Your summary at the end. Breaking change for default + a deprecation saying that `numeric_only` will be removed entirely.

> Personally, I might have a slight preference to actually do a breaking change on this for 1.0 for the default behaviour (we still need to deprecate the numeric_only keyword when specified, that's a separate thing). But, it's certainly possible to do it with a deprecation (we will only need to change skipna=True into skipna=None to know when it was explicitly specified by the user).
can you merge master.
It is strange that the failed tests don't call `categorical.min` or `categorical.max`.
@makbigc those are indeed unrelated, so you can ignore those for now (they are being fixed in https://github.com/pandas-dev/pandas/pull/29877)
@makbigc I merged that other PR. So if you merge latest master in this branch, the error should be solved.
@jreback anything else? Please tell me.  #27801 is pending for it
@jreback I opened an issue for your remaining comment about always returning NaN regardless of the dtype: https://github.com/pandas-dev/pandas/issues/29962