Value counts normalize
State: closed

- [x] closes #25970
- [x] closes #32471
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This pull request resolves issues with binning and NA values in both ``Series.value_counts`` and ``SeriesGroupBy.value_counts,`` adding new tests to check the problematic cases.

Labels:
name: Bug
name: Groupby
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
name: Stale

Comments:
Hello @DataInformer! 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 2020-09-14 02:11:04 UTC
I'm not sure why Web and docs is failing.  Looking through the output, I only see warnings (for my part, only about block quote issue that I think is detecting on ``pd.cut``
I'm not positive but it could have something to do with https://pandas.pydata.org/docs/development/contributing_docstring.html
```
/home/runner/miniconda3/envs/pandas-dev/lib/python3.8/site-packages/IPython/sphinxext/ipython_directive.py:1023: UserWarning: Code input with no code at /home/runner/work/pandas/pandas/doc/source/user_guide/computation.rst, line 622
  warnings.warn(message)
/home/runner/work/pandas/pandas/pandas/core/base.py:docstring of pandas.Index.value_counts:18: WARNING: Block quote ends without a blank line; unexpected unindent.
/home/runner/work/pandas/pandas/pandas/core/base.py:docstring of pandas.Series.value_counts:18: WARNING: Block quote ends without a blank line; unexpected unindent.
```
```
build finished with problems, 2 warnings.
##[error]Process completed with exit code 1.
```
> I'm not positive but it could have something to do with https://pandas.pydata.org/docs/development/contributing_docstring.html
> 
> ```
> /home/runner/miniconda3/envs/pandas-dev/lib/python3.8/site-packages/IPython/sphinxext/ipython_directive.py:1023: UserWarning: Code input with no code at /home/runner/work/pandas/pandas/doc/source/user_guide/computation.rst, line 622
>   warnings.warn(message)
> /home/runner/work/pandas/pandas/pandas/core/base.py:docstring of pandas.Index.value_counts:18: WARNING: Block quote ends without a blank line; unexpected unindent.
> /home/runner/work/pandas/pandas/pandas/core/base.py:docstring of pandas.Series.value_counts:18: WARNING: Block quote ends without a blank line; unexpected unindent.
> ```
> 
> ```
> build finished with problems, 2 warnings.
> ##[error]Process completed with exit code 1.
> ```



> I'm not positive but it could have something to do with https://pandas.pydata.org/docs/development/contributing_docstring.html
> 
> ```
> /home/runner/miniconda3/envs/pandas-dev/lib/python3.8/site-packages/IPython/sphinxext/ipython_directive.py:1023: UserWarning: Code input with no code at /home/runner/work/pandas/pandas/doc/source/user_guide/computation.rst, line 622
>   warnings.warn(message)
> /home/runner/work/pandas/pandas/pandas/core/base.py:docstring of pandas.Index.value_counts:18: WARNING: Block quote ends without a blank line; unexpected unindent.
> /home/runner/work/pandas/pandas/pandas/core/base.py:docstring of pandas.Series.value_counts:18: WARNING: Block quote ends without a blank line; unexpected unindent.
> ```
> 
> ```
> build finished with problems, 2 warnings.
> ##[error]Process completed with exit code 1.
> ```

Right, that's what I thought, but I don't see any block quotes without blank lines.  I was hoping maybe someone could help me identify more specifically what the problem is.
> Is the fix for #32471 dependent on the fix for #25970? If so is it possible to address #25970 independently as a pre-cursor PR?

I cleaned up the pull request so it has minimal changes only addressing #25970.  I kept the branch name so this request history would remain.
> > Is the fix for #32471 dependent on the fix for #25970? If so is it possible to address #25970 independently as a pre-cursor PR?
> 
> I cleaned up the pull request so it has minimal changes only addressing #25970. I kept the branch name so this request history would remain.

However, I had only run the base tests, and the groupby tests now cause conflicting results, since the Series.value_count is doing the right thing but the DataFrame.groupby.value_count is not.  It seems wrong to change the test, but I have temporarily reduced the normalize and dropna parameters to skip the problematic cases.  Would you rather I combine the fixes again as a single pull request?
This passes tests, but does show a drop in performance for the groupby value_counts.  I'm trying to see how to improve that, though a small gap may be needed to achieve correct results.
I'm still a bit confused by the results from the benchmarking (pasted below).  In particular, I don't see how my changes would have made some of the methods faster, when I didn't change those, which casts doubt on the results showing that value_counts is significantly slower.  My code handles a couple special cases separately to avoid the identified bugs, but I would not expect it to be more than 20% slower.  
This run is with the computer idle (I get very different results while still using it, which makes sense).  I reached out to the Pandas-dev alias to see if someone could advise on the performance issues, but didn't get any response.  Do you have other suggestions for tracking down what might be causing these methods to run slower?


'asv continuous -f 1.1 upstream/master HEAD -b groupby.GroupByMethods'
       before           after         ratio
     [04e9e0af]       [5abfb164]
     <value_counts_part1~1^2>       <value_counts_normalize>
+     1.03±0.03ms      1.85±0.05ms     1.80  groupby.GroupByMethods.time_dtype_as_group('float', 'value_counts', 'transformation')
+     1.03±0.06ms       1.81±0.1ms     1.75  groupby.GroupByMethods.time_dtype_as_group('int', 'value_counts', 'transformation')
+     1.02±0.07ms      1.70±0.06ms     1.67  groupby.GroupByMethods.time_dtype_as_field('int', 'value_counts', 'transformation')
+      1.01±0.1ms      1.65±0.07ms     1.64  groupby.GroupByMethods.time_dtype_as_group('float', 'value_counts', 'direct')
+     1.05±0.04ms       1.64±0.2ms     1.56  groupby.GroupByMethods.time_dtype_as_group('datetime', 'value_counts', 'transformation')
+     1.08±0.08ms      1.67±0.09ms     1.55  groupby.GroupByMethods.time_dtype_as_group('datetime', 'value_counts', 'direct')
+     1.11±0.04ms      1.69±0.06ms     1.52  groupby.GroupByMethods.time_dtype_as_field('int', 'value_counts', 'direct')
+        962±20μs      1.41±0.03ms     1.47  groupby.GroupByMethods.time_dtype_as_field('object', 'value_counts', 'direct')
+     1.09±0.04ms      1.55±0.08ms     1.42  groupby.GroupByMethods.time_dtype_as_field('object', 'value_counts', 'transformation')
+     1.21±0.02ms      1.69±0.05ms     1.40  groupby.GroupByMethods.time_dtype_as_field('datetime', 'value_counts', 'transformation')
+     1.17±0.06ms      1.64±0.06ms     1.40  groupby.GroupByMethods.time_dtype_as_group('int', 'value_counts', 'direct')
+     1.28±0.09ms       1.72±0.1ms     1.35  groupby.GroupByMethods.time_dtype_as_field('float', 'value_counts', 'transformation')
+     1.23±0.03ms      1.66±0.06ms     1.34  groupby.GroupByMethods.time_dtype_as_field('datetime', 'value_counts', 'direct')
+     1.31±0.08ms       1.71±0.1ms     1.30  groupby.GroupByMethods.time_dtype_as_field('float', 'value_counts', 'direct')
+         191±9μs          241±9μs     1.26  groupby.GroupByMethods.time_dtype_as_group('object', 'head', 'transformation')
+        865±50μs      1.08±0.05ms     1.25  groupby.GroupByMethods.time_dtype_as_group('object', 'value_counts', 'transformation')
+        947±20μs      1.17±0.06ms     1.24  groupby.GroupByMethods.time_dtype_as_group('object', 'value_counts', 'direct')
+        577±30μs         684±20μs     1.18  groupby.GroupByMethods.time_dtype_as_field('int', 'min', 'direct')
+        595±30μs         698±30μs     1.17  groupby.GroupByMethods.time_dtype_as_field('float', 'quantile', 'transformation')
-        703±70μs         635±30μs     0.90  groupby.GroupByMethods.time_dtype_as_group('datetime', 'min', 'direct')
-         287±7μs         255±10μs     0.89  groupby.GroupByMethods.time_dtype_as_group('float', 'cumcount', 'direct')
-     1.26±0.04ms      1.10±0.07ms     0.87  groupby.GroupByMethods.time_dtype_as_group('int', 'cumprod', 'direct')
-        237±10μs          206±2μs     0.87  groupby.GroupByMethods.time_dtype_as_group('float', 'head', 'direct')
-         183±6μs         158±10μs     0.86  groupby.GroupByMethods.time_dtype_as_field('float', 'count', 'direct')
-        516±20μs         440±20μs     0.85  groupby.GroupByMethods.time_dtype_as_field('object', 'nunique', 'direct')
-        993±20μs         835±10μs     0.84  groupby.GroupByMethods.time_dtype_as_group('float', 'sem', 'direct')
@DataInformer is this still active? If so can you merge master and try to get CI green?
> @DataInformer is this still active? If so can you merge master and try to get CI green?

Yes, this is active.  I merged master again and checks pass now.  Hopefully we're good to close this out.
> wow this is a huge amount of change. It would be really nice to do this in smaller pieces, otherwise review is going to take an extended amount of time.

I tried to do this request in pieces (see June 27 comments), but because the existing tests compare the results of Series.value_counts with SeriesGroupby.value_counts, it was impossible to fix only one of these and still have tests pass.
I know it's a lot to go through, but is there a time you think you'll get to the full review?  I can merge master again when you're ready.
> I'm still a bit confused by the results from the benchmarking (pasted below). In particular, I don't see how my changes would have made some of the methods faster, when I didn't change those, which casts doubt on the results showing that value_counts is significantly slower.

It is, unfortunately, normal to get a lot of noise in asv results.  Two main ways to handle it: 1) re-run and manually discard results that are not consistent(ish) across runs 2) use %timeit on targeted snippets

@jreback @jbrockmendel @WillAyd @simonjayhawkins is there someone who could have a 5min phone conversation with me about this PR and how to make it easier to evaluate?  858-205-8203
Thanks,
Evan
@DataInformer i won't have time to look at this for at least a few weeks

we just have quite a backlog

if @jbrockmendel can look in depth would be great 
Mind merging in master and resolving conflicts? It would help the review process
> Mind merging in master and resolving conflicts? It would help the review process

I'm happy to do so when you all are ready for a full review.  Is that time now?
It will be easier for the core devs to perform a full review with an updated pull request: https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#tips-for-a-successful-pull-request
> It will be easier for the core devs to perform a full review with an updated pull request: https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#tips-for-a-successful-pull-request

I definitely want to make things easy for the devs, but I've gotten very sporadic feedback about this PR and I prefer not to repeatedly merge dev or respond to comments made weeks apart.  What can I do to make this easier to review while also being reasonable for me?
Sorry for the sporadic feedback; since pandas is a community project, the core devs' time commits can be all over the place.

The best approach would be to keep the PR up to date with master and ping the devs that have responded already to this PR when you're ready for another review.
@DataInformer Thanks for the PR. closing as stale. ping if you want to continue and will reopen