BUG: improve handling of Series.value_counts's argument 'dropna' (GH9443)
State: closed

closes #9443 


Labels:
name: Bug
name: Categorical
description: Categorical Data Type

Comments:
If `Categorical.dropna` looks good, I could add tests for it, too, of course.

Is it needed that `dropna` and `value_counts` are added as public functions to `Categorical`? 
In my ming, this goes a bit against the reasoning of keeping `Categorical` an array like (as the 'real' numpy array does not support such dtype).

@jorisvandenbossche I don't see any problem with having these functions. They can be public or not, doesn't make much difference. It does not matter that numpy doesn't support these. It likely never will.

`Categorical` acts array-like, but so what.

@jreback, are you waiting for anything else from me, or are we good to go?

@jorisvandenbossche 
cc @Janschulz

so adding `.value_counts/.dropna` to `Categorical`, really to make the calling code simpler. Question is do we / should we expose these as public-ish method, ala `.cat` ?

@Kodiologist ~~pls add `.value_counts/.dropna` to API.rst (in Categorical section).~~

@jreback, @Kodiologist: I think these two methods should neither be added to `Series.cat` nor to api.rst: the categorical section in api.rst is on purpose only the methods under `Series.cat` and these methods only make sense on a categorical but not on any other dtype. In this case it is simple that the implementation of a method in `Series` is different for `Categorical` than `ndarray` and the code is put into `Categorical` instead of making the method longer. During the initial iterations, many methods from Categorical, which I had added to api.rst, were removed in the end for that reason: min,max,... 

I think public is fine, as long as the API documentation makes it clear that directly using methods on `Categorical` or via `s.values` is prone to break over the time (currently API: everything under `s.cat` and the `Categorical` constructor). If these should become private, it makes sense to also mark all other methods (min, max,...) private and I don't think that's possible (some are used as a drop-in for public `ndarray` methods, so the caller would need to test for `_min` and `min`...)

> I think public is fine, as long as the API documentation makes it clear that directly using methods on Categorical or via `s.values` is prone to break over the time

Lets just make them internal then, no? Then we don't have that potential problem.

So to say I still vote for making them private functions. No user should ever use this I think? As we want them to use the categorical series, and not the raw Categorical. So a user should use `Series.dropna()`, and not `Categorical.dropna()` (there is also not a `ndarray.dropna()`). Providing it (even when documenting not to use it), will just tempt user to use it. 
And in an ideal world, it is still the goal that one day an array with a native `Categorical` type will be available (if not a numpy array, then maybe a dynd array), and in that case we want to replace `Categorical` with that I think (or at least the user facing `.values` categorical). So, until then, I would keep the public API of `Categorical` as much as close to a plain ndarray, and let the special functions it has be exposed for users through `.cat`

Note the changes to test_categorical. 0 counts and frequencies for unused categories seem more correct to me than NA counts and frequencies. Also, I removed one test entirely, which had a Categorical with both NA codes (-1) and an NA category, because that seems like a situation that shouldn't be allowed. We could enforce that rule, but I think that as things stand, the user needs to do some pretty silly things to get a Categorical like that, like renaming an existing level to `np.nan` while there are already NAs (-1 codes) in the Categorical, so enforcement may not be necessary.

@jreback, looks good?

@Kodiologist some minor comments, mostly stylist. Pls rebase and squash to a single commit. If you can do this today I wll put in 0.16.0

Sure thing.

@jreback All done.

merged via https://github.com/pydata/pandas/commit/0270484ed9231349475d280526d9c58698c3475d

@Kodiologist thanks!