BUG: Fix passing of numeric_only argument for categorical reduce
State: closed

See https://github.com/pandas-dev/pandas/issues/25303 and https://github.com/pandas-dev/pandas/issues/25299

- [x] closes https://github.com/pandas-dev/pandas/issues/25299
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Regression
description: Functionality that used to work in a prior pandas version
name: Categorical
description: Categorical Data Type

Comments:
Hello @arnov! Thanks for updating the PR.







Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on February 15, 2019 at 12:32 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=h1) Report
> Merging [#25304](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b8306f19da098a826f621022ddce279119de2f95?src=pr&el=desc) will **decrease** coverage by `49.99%`.
> The diff coverage is `0%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25304/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25304      +/-   ##
==========================================
- Coverage   91.72%   41.72%     -50%     
==========================================
  Files         173      173              
  Lines       52831    52831              
==========================================
- Hits        48457    22042   -26415     
- Misses       4374    30789   +26415
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.72% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `45.47% <0%> (-48.22%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| ... and [130 more](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=footer). Last update [b8306f1...ab9f597](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=h1) Report
> Merging [#25304](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/53281a5962e75655cb3e10cbf290234655a359e1?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25304/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25304      +/-   ##
==========================================
+ Coverage   91.72%   91.72%   +<.01%     
==========================================
  Files         173      173              
  Lines       52831    52833       +2     
==========================================
+ Hits        48457    48459       +2     
  Misses       4374     4374
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.28% <100%> (ø)` | :arrow_up: |
| #single | `41.72% <75%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.97% <100%> (ø)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/25304/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.7% <100%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=footer). Last update [53281a5...ec1fa9f](https://codecov.io/gh/pandas-dev/pandas/pull/25304?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Thanks for the PR! 
See my comment here https://github.com/pandas-dev/pandas/issues/25299#issuecomment-463394298, I think we should separately try to fix it using `numeric_only` (would you want to do that here?), and then think about deprecating it (see https://github.com/pandas-dev/pandas/issues/25303)
@arnov Thanks for the updates! 
But, as I said above, I think it would be better to keep the numeric_only fix, and the deprecation separate. The deprecation is only for 0.25.0, but the regression fix would be good to include in version 0.24.2
@jorisvandenbossche yes, sorry! I've made this one simpler and kept only the fix for numeric_only. I've kept the other changes in another branch, so could make a separate PR out of that after this one is done.
Two further questions:

- can you add a note in the 0.24.2 whatsnew file? (in the regression section)
- workflow related: in general, we prefer that you push new commits to a PR, instead of squashing/amending a previous commit and force pushing. That way it makes it easier to eg see what changed since a last review (in this case not that important of course since it is a very small diff), and we squash/merge PRs in the end anyway, so it is no problem that there are multiple (unclean) commits in the PR branch.
Moving this discussion out of the review comment (https://github.com/pandas-dev/pandas/pull/25304#discussion_r257208313) to keep it easier to follow. 
Discussion is about where to add the special casing for the `numeric_only` keyword for Categorical: in `Series._reduce` or in `Categorical`. Adding it in `Series._reduce` is less clean (but only temporary), but moving it to `Categorical` would mean that the `EA._reduce` in general needs to accept (and ignore) the `numeric_only`. Which would be an API change.

But to take a step back: @jreback do you agree that in the _long term_ we can deprecate this `numeric_only` keyword for Categorical.min/max?
(it's the only EA that now uses it, while the others all use `skipna` for the same thing)
> But to take a step back: @jreback do you agree that in the _long term_ we can deprecate this `numeric_only` keyword for Categorical.min/max?
(it's the only EA that now uses it, while the others all use `skipna` for the same this

not sure what you mean here. The ``numeric_only`` kwarg is pretty generic an ingrained. I think you need to accept it for ``Catetogorical`` and either ignore it or handle it. EA's can choose to respond to this or not. But this is a higher level operation, applying to each array in turn.
Again, please read  #25303, and respond there is you don't agree or don't understand the reasoning. This is *not* about the general `numeric_only` keyword.


OK, so we agree that, in the long term, we want to deprecate and remove `numeric_only` from `Categorical.min/max`.

To fix the regression (what this PR is doing), we still have to deal with this special case until it is finally removed. 
So either we need to *change* the public API of `EA._reduce`, or either we agree to have the special case in `Series._reduce` (we are talking about *two lines* of code), which is what this PR is doing.

Changing the public API will lead to more changes internally than 2 lines, but moreover is something we should IMO not do for a bug fix release. So @jreback short-term proposal: we do it as in this PR for 0.24.2, and discuss / re-work it for 0.25.0 ?
> Changing the public API will lead to more changes internally than 2 lines, but moreover is something we should IMO not do for a bug fix release. So @jreback short-term proposal: we do it as in this PR for 0.24.2, and discuss / re-work it for 0.25.0 

that's extra work, just mark this for 0.25.0 and let's fix properly. I don't think this is very hard to fix anyhow. ``_reduce`` already accepts kwargs IIRC.
@jreback can we move the discussion to gitter if you have a moment?
thanks @arnov