PERF: Cythonize Groupby Rank
State: closed

- [x] closes #15779
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

~~This is not complete~~ but I wanted to submit for review on the direction. In particular, I wanted to know if my way of passing the named rank arguments back to the Cython layer makes sense, or if we'd rather bypass using kwargs and call that function directly from the `GroupBy` instance method (similar to how `shift` does it).

Right now this only increments values ascending, doesn't handle tiebreakers, nor does it allow for the return of a percentage. I also plan on adding some test cases to cover the arguments as I can't find them in the `pandas.tests.groupby` package. More to come but again wanted feedback before going too far in

Labels:
name: Groupby
name: Performance
description: Memory or execution speed performance

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





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

##### Comment last updated on February 09, 2018 at 18:37 Hours UTC
Still not complete but sharing in interim for any code review as this change is large. At a minimum I need to go back and use something besides kwargs to pass the appropriate values back
Benchmarks provided below for reference

```bash
Before     after       ratio
  [f391cbfe] [27cb6b57]
+  139.69μs   535.43μs      3.83  groupby.GroupByMethods.time_method('float', 'cummin')
+  142.06μs   522.73μs      3.68  groupby.GroupByMethods.time_method('float', 'cummax')
+  145.02μs   527.11μs      3.63  groupby.GroupByMethods.time_method('int', 'cummin')
+  162.18μs   574.63μs      3.54  groupby.GroupByMethods.time_method('float', 'cumsum')
+  145.86μs   512.28μs      3.51  groupby.GroupByMethods.time_method('int', 'cummax')
+  159.64μs   533.92μs      3.34  groupby.GroupByMethods.time_method('int', 'cumsum')
+  311.50μs   728.59μs      2.34  groupby.GroupByMethods.time_method('float', 'cumprod')
+  315.28μs   650.14μs      2.06  groupby.GroupByMethods.time_method('int', 'cumprod')
+  872.00μs     1.24ms      1.42  groupby.GroupByMethods.time_method('int', 'sem')
+  901.00μs     1.21ms      1.34  groupby.GroupByMethods.time_method('float', 'sem')
+  161.36μs   203.44μs      1.26  groupby.GroupByMethods.time_method('int', 'min')
+  200.87ms   235.93ms      1.17  groupby.GroupByMethods.time_method('float', 'all')
+   64.93μs    75.73μs      1.17  groupby.GroupByMethods.time_method('int', 'size')
+  299.42μs   345.95μs      1.16  groupby.GroupByMethods.time_method('int', 'std')
+  201.29ms   231.93ms      1.15  groupby.GroupByMethods.time_method('float', 'any')
+  579.50ms   659.22ms      1.14  groupby.GroupByMethods.time_method('int', 'pct_change')
+     2.84s      3.19s      1.12  groupby.GroupByMethods.time_method('float', 'describe')
+  192.43μs   215.57μs      1.12  groupby.GroupByMethods.time_method('int', 'cumcount')
+  319.90μs   356.71μs      1.12  groupby.GroupByMethods.time_method('int', 'median')
+  156.03μs   173.91μs      1.11  groupby.GroupByMethods.time_method('float', 'min')
+  121.26μs   134.29μs      1.11  groupby.GroupByMethods.time_method('int', 'shift')
+  173.37μs   191.73μs      1.11  groupby.GroupByMethods.time_method('float', 'var')
-  193.48ms   795.71μs      0.00  groupby.GroupByMethods.time_method('int', 'rank')
-  301.72ms   787.03μs      0.00  groupby.GroupByMethods.time_method('float', 'rank')

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
```
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19481?src=pr&el=h1) Report
> Merging [#19481](https://codecov.io/gh/pandas-dev/pandas/pull/19481?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/a214915e241ea15f3d072d54930d0e0c8f42ee10?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #19481      +/-   ##
==========================================
+ Coverage   91.59%   91.62%   +0.02%     
==========================================
  Files         150      150              
  Lines       48795    48803       +8     
==========================================
+ Hits        44696    44715      +19     
+ Misses       4099     4088      -11
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.99% <100%> (+0.02%)` | :arrow_up: |
| #single | `41.72% <33.33%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19481?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/19481/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.2% <100%> (+0.07%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/19481/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `83.64% <0%> (-0.21%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/19481/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `66.95% <0%> (+1.73%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19481?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/19481?src=pr&el=footer). Last update [a214915...aa4578d](https://codecov.io/gh/pandas-dev/pandas/pull/19481?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I posted AVSs for a previous commit. Trying to repost for latest but they aren't working any more. I think 3597de0ada3869c8841d05ef4353195a6e124184 broke the ASVs by removing `rolling_median` and potentially other functions, which now yields the following trying to run benchmarks:

```bash
File "/Users/williamayd/Git/pandas/asv_bench/benchmarks/gil.py", line 3, in <module>
  from pandas import (DataFrame, Series, rolling_median, rolling_mean,
ImportError: cannot import name 'rolling_median'
```

Need to debug a little more but will probably have to open a separate issue to fix and get those working again

EDIT: looks like #19236 is supposed to fix the ASV issue. Can repost results after that gets merged
Latest ASVs posted below

```bash

       before           after         ratio
     [b8351277]       [0141747d]
+         145±2μs          479±2μs     3.29  groupby.GroupByMethods.time_method('int', 'cummax')
+       144±0.8μs          469±6μs     3.26  groupby.GroupByMethods.time_method('int', 'cummin')
+         148±2μs          479±8μs     3.24  groupby.GroupByMethods.time_method('float', 'cummax')
+       160±0.9μs         493±10μs     3.08  groupby.GroupByMethods.time_method('float', 'cumsum')
+         155±6μs         478±20μs     3.08  groupby.GroupByMethods.time_method('float', 'cummin')
+         169±2μs          484±1μs     2.87  groupby.GroupByMethods.time_method('int', 'cumsum')
+         557±3μs       1.14±0.2ms     2.05  groupby.GroupByMethods.time_method('int', 'sem')
+         308±5μs          626±2μs     2.03  groupby.GroupByMethods.time_method('int', 'cumprod')
+         353±8μs          636±5μs     1.80  groupby.GroupByMethods.time_method('float', 'cumprod')
+       149±0.7μs          192±1μs     1.29  groupby.GroupByMethods.time_method('float', 'min')
+         300±2μs         380±10μs     1.27  groupby.GroupByMethods.time_method('float', 'head')
+         312±7μs          385±2μs     1.24  groupby.GroupByMethods.time_method('int', 'std')
+         149±1μs          180±8μs     1.21  groupby.GroupByMethods.time_method('float', 'last')
+         335±2μs          403±7μs     1.20  groupby.GroupByMethods.time_method('float', 'median')
+       353±0.9μs         420±10μs     1.19  groupby.GroupByMethods.time_method('int', 'nunique')
+         484±5μs         570±20μs     1.18  groupby.GroupByMethods.time_method('float', 'sem')
+           914ms            1.04s     1.14  groupby.GroupByMethods.time_method('float', 'mad')
+           889ms            1.00s     1.13  groupby.GroupByMethods.time_method('float', 'pct_change')
+           2.93s            3.28s     1.12  groupby.GroupByMethods.time_method('float', 'describe')
+         214±4ms         237±10ms     1.11  groupby.GroupByMethods.time_method('int', 'skew')
-           1.98s            1.77s     0.89  groupby.GroupByMethods.time_method('int', 'describe')
-         178±5μs        157±0.7μs     0.89  groupby.GroupByMethods.time_method('int', 'first')
-       187±0.7ms         772±20μs     0.00  groupby.GroupByMethods.time_method('int', 'rank')
-       293±0.9ms         793±40μs     0.00  groupby.GroupByMethods.time_method('float', 'rank')

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
```
Missed your note on #11759 before latest commit. With the example provided there, this fix would now be raising `TypeError: 'NoneType' object is not callable` because there is no `group_rank_object` implementation - is that what you are looking for and if so do you not feel that is covered already by `test_rank_object_raises` method that was added?
thanks! ok ideally have a better message for unsupported groupbys, but can address then with a followup.
https://travis-ci.org/MacPython/pandas-wheels/jobs/340767780 is a 32-bit build which is failing

sample error


```
/venv/lib/python3.6/site-packages/pandas/tests/groupby/test_groupby.py:1932: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:1804: in rank
    na_option=na_option, pct=pct, axis=axis)
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:1006: in _cython_transform
    **kwargs)
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:2427: in transform
    return self._cython_operation('transform', values, how, axis, **kwargs)
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:2387: in _cython_operation
    **kwargs)
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:2461: in _transform
    transform_func(result, values, comp_ids, is_datetimelike, **kwargs)
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:2276: in wrapper
    return f(afunc, *args, **kwargs)
/venv/lib/python3.6/site-packages/pandas/core/groupby.py:2228: in <lambda>
    kwargs.get('na_option', 'keep')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
>   ???
E   ValueError: Buffer dtype mismatch, expected 'int64_t' but got 'int'
pandas/_libs/groupby_helper.pxi:1250: ValueError
```

not sure exactly what is happening, if you can have a look. note that it is almost impossible to actually test this w/o merging. these are built once a day. once we have a fix which passes regular travis then can go from there. thanks.
**EDIT**: Ignore below comment - didn't look closely enough at traceback you provided where `rank` is the offending function

Judging by the line number I think this may be a side effect of #19635. Previously there was the following condition preceding that:

```python
  {{if name == 'int64'}}		
  if val != {{nan_val}}:		
  {{else}}		
  if val == val and val != {{nan_val}}:
  {{endif}}
```

Which I simplified to

```python
  if val == val and val != {{nan_val}}:
```

I can't imagine why that would make a difference but it is suspicious given location of the error. Want me to add that condition back in and submit it referencing this PR?
yep thanks


@WillAyd not sure if it is related to this PR, but on AppVeyor, there are a huge amount of numpy warnings ("DeprecationWarning: numpy boolean subtract, the `-` operator, is deprecated, use the bitwise_xor, the `^` operator, or the logical_xor function instead.") related to `rank` tests (so many warnings you even don't see the actual errors anymore). 

See https://ci.appveyor.com/project/pandas-dev/pandas/build/1.0.13483/job/vt1rpmyub41gf6aa
From a quick manual appveyor bisect: introduced by https://github.com/pandas-dev/pandas/pull/20091
cc @peterpanmj 
I think this warning is coming from `np.diff` specifically at the below line (link goes to master):

https://github.com/numpy/numpy/blob/a9e9343ecf5a6564eb09f1d7078c407db2ff7d9e/numpy/lib/function_base.py#L1172

It looks like in 1.13 this was actually performing subtraction regardless of type, so perhaps something that just slipped through on that release?
Yes, this is a result of np.diff in 1.13.  I use np.dfiff in #20091. Actually something like ^ operator can be used instead.