BUG: Fix passing of numeric_only argument for categorical reduce

[arnov]

See #25303 and #25299

• closes Minimum of ordered categorical data in Panda DataFrames #25299
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @arnov! Thanks for updating the PR.

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

Comment last updated on February 15, 2019 at 12:32 Hours UTC

[codecov]

Codecov Report

Merging #25304 into master will decrease coverage by 49.99%.
The diff coverage is 0%.

@@            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%)	⬇️

Impacted Files	Coverage Δ
pandas/core/series.py	45.47% <0%> (-48.22%)	⬇️
pandas/io/formats/latex.py	0% <0%> (-100%)	⬇️
pandas/core/categorical.py	0% <0%> (-100%)	⬇️
pandas/io/sas/sas_constants.py	0% <0%> (-100%)	⬇️
pandas/tseries/plotting.py	0% <0%> (-100%)	⬇️
pandas/tseries/converter.py	0% <0%> (-100%)	⬇️
pandas/io/formats/html.py	0% <0%> (-99.35%)	⬇️
pandas/core/groupby/categorical.py	0% <0%> (-95.46%)	⬇️
pandas/io/sas/sas7bdat.py	0% <0%> (-91.17%)	⬇️
pandas/io/sas/sas_xport.py	0% <0%> (-90.15%)	⬇️
... and 130 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8306f1...ab9f597. Read the comment docs.

[codecov]

Codecov Report

Merging #25304 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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%> (ø)	⬆️
#single	41.72% <75%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/categorical.py	95.97% <100%> (ø)	⬆️
pandas/core/series.py	93.7% <100%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53281a5...ec1fa9f. Read the comment docs.

[jorisvandenbossche]

Thanks for the PR!
See my comment here #25299 (comment), 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 #25303)

[jorisvandenbossche]

@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

[arnov]

@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.

[jorisvandenbossche]

Thanks a lot!

Added some minor comments for the tests

[jorisvandenbossche]

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.

[jorisvandenbossche]

Great! Looks all good to me.

I will let it open a bit longer in case others have comments.

[jreback]

why are u adding a code path here? the original is much more generic ; need to avoid special cases like this
if u need to handle this specially then the place is in the Categirical itself

[jorisvandenbossche]

This is because Categorical deviates here from the standard ExtensionArray (see #25303 for the issue about that).
I personally find it clearer with this special case, making it explicit that Categorical has a different signature. And after the deprecation period, we can remove this special case.

If you feel strongly about it, it can indeed be handled inside Categorical. But that means that all the other arrays' _reduce method needs to be updated as well to handle (=ignore) numeric_only, which is also not clean (and the special case here is only temporarily anyway).

[jreback]

this is not more clear and leads to future issues
pls move to _reduce

[jreback]

not sure why the others need to change at all you’re logic is circular

[arnov]

This would cause more changes, since the problem is actually that the argument numeric_only is not passed currently. If we would add it for every ExtensionArray call we get problems at the other reduction methods. For instance here: https://github.com/pandas-dev/pandas/blob/master/pandas/core/arrays/numpy_.py#L322 (they don't have numeric_only, or **kwargs in the method definition).

So we could change the call for every ExtenensionArray to:

return delegate._reduce(name, skipna=skipna, numeric_only=numeric_only, **kwds)

but then we would need to make sure every child of ExtensionArray supports this and this is currently not the case.

[jorisvandenbossche]

@arnov explained it well. I think we don't want to change the EA interface (_reduce is an official part of it) just for this back-compat special case we are going to deprecate. Hence, categorical needs to be handled separately here (but again, this is only temporary)

[jreback]

disagree numeric_only is likely not going away anytime soon and even so
the EA simply need to accept it (they can ignore it)

[jorisvandenbossche]

@jreback this is not about the numeric_only in DataFrame/Series reductions that determines for which columns the reduction is calculated. This is another meaning of the keyword only for categorical that determines whether NaNs should be skipped or not. Please read #25303

So we are not speaking about removing that general use case of numeric_only, but only the one in Categorical.min/max.

[jreback]

i’ll look closer but am still -1 on any handling as a special case in the Series call
the point is that pass on kwargs; EA can ignore or not as required

[jorisvandenbossche]

OK, 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)

[jorisvandenbossche]

Moving this discussion out of the review comment (#25304 (comment)) 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)

[jreback]

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.

[jorisvandenbossche]

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.

[jorisvandenbossche]

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 ?

[jreback]

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.

[jorisvandenbossche]

@jreback can we move the discussion to gitter if you have a moment?

[jreback]

thanks @arnov