Cythonized GroupBy Fill
State: closed

- [X] closes #11296
- [X] tests added / passed
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

I am not a fan of how I've implemented this in `groupby.py` but I think this change highlights even more the need for some refactoring of that module, specifically in how Cython transformations are getting dispatched. This still works in the meantime but open to any feedback on how the methods are getting wired back to the Cython layer.

Below are ASVs for the change

```bash
       before           after         ratio
     [d9551c8e]       [5e007f86]
+      81.6±0.3μs         96.7±2μs     1.19  groupby.GroupByMethods.time_method('float', 'count')
+         455±3μs         516±20μs     1.13  groupby.GroupByMethods.time_method('float', 'cummin')
+       306±0.6μs          340±5μs     1.11  groupby.GroupByMethods.time_method('float', 'prod')
-       142±0.4ms          280±6μs     0.00  groupby.GroupByMethods.time_method('int', 'bfill')
-        230±10ms          327±7μs     0.00  groupby.GroupByMethods.time_method('float', 'bfill')
-       135±0.8ms        189±0.3μs     0.00  groupby.GroupByMethods.time_method('int', 'ffill')
-         227±2ms        184±0.6μs     0.00  groupby.GroupByMethods.time_method('float', 'ffill')
```

Labels:
name: Groupby
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Performance
description: Memory or execution speed performance

Comments:
One possibility to clean this up would be to replicate `GroupBy.shift`
https://github.com/pandas-dev/pandas/blob/07137a5acdfc761c7cc30a081600e4c4f23c7d3d/pandas/core/groupby.py#L1846

Rather than type specific functions, there the cython routine computes a single indexer, then the normal `take` functions are used to make the actual output.
https://github.com/pandas-dev/pandas/blob/07137a5acdfc761c7cc30a081600e4c4f23c7d3d/pandas/_libs/groupby_helper.pxi.in#L991

Probably a bit slower than what you do here, but would be surprised if it's bad.

Thanks for the idea. I think I could make something like that work here and that would definitely simplify things
Just simplified the code to mirror what we do for `shift`. The `groupby.py` changes are MUCH cleaner now and there doesn't appear to be any significant change to benchmarks either. Updated results below:

```bash
       before           after         ratio
     [07137a5a]       [0939aeae]
+           983ms            1.23s     1.25  groupby.GroupByMethods.time_method('float', 'pct_change')
+         356±4μs         443±20μs     1.24  groupby.GroupByMethods.time_method('float', 'prod')
+      84.6±0.2μs          101±3μs     1.19  groupby.GroupByMethods.time_method('int', 'count')
+         185±1μs          218±3μs     1.18  groupby.GroupByMethods.time_method('int', 'cumcount')
+         161±3μs          185±3μs     1.15  groupby.GroupByMethods.time_method('float', 'cumcount')
+         334±9μs         382±20μs     1.14  groupby.GroupByMethods.time_method('float', 'median')
+         461±3μs         523±10μs     1.13  groupby.GroupByMethods.time_method('int', 'cummax')
+       344±0.8μs         387±10μs     1.12  groupby.GroupByMethods.time_method('float', 'nunique')
+      64.5±0.2μs       72.1±0.9μs     1.12  groupby.GroupByMethods.time_method('int', 'size')
+         456±2μs         504±20μs     1.11  groupby.GroupByMethods.time_method('float', 'cummin')
+         133±5ms          146±3ms     1.10  groupby.GroupByMethods.time_method('int', 'any')
-        769±20μs         684±30μs     0.89  groupby.GroupByMethods.time_method('int', 'cumprod')
-       893±100μs         598±30μs     0.67  groupby.GroupByMethods.time_method('int', 'sem')
-         145±2ms          275±7μs     0.00  groupby.GroupByMethods.time_method('int', 'bfill')
-         154±3ms          245±1μs     0.00  groupby.GroupByMethods.time_method('int', 'ffill')
-         228±5ms         270±10μs     0.00  groupby.GroupByMethods.time_method('float', 'bfill')
-         237±5ms        232±0.5μs     0.00  groupby.GroupByMethods.time_method('float', 'ffill')
```
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19673?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@feedf66`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff            @@
##             master   #19673   +/-   ##
=========================================
  Coverage          ?   91.65%           
=========================================
  Files             ?      150           
  Lines             ?    48962           
  Branches          ?        0           
=========================================
  Hits              ?    44875           
  Misses            ?     4087           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.03% <100%> (?)` | |
| #single | `41.8% <17.24%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19673?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/19673/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.31% <100%> (ø)` | |

------

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

Latest commit includes a shared function between `fill` and `shift`, which can theoretically house `any` / `all` implementations with a little clean up as well. As touched on in previous conversations, if we expand this it should probably be in its own module

Open to review and plan to change the location of the func within the module / update docs, but I wanted to push because this commit will cause tests to fail at the following location:

https://github.com/pandas-dev/pandas/blob/718d0679600fa025872d8b66c198a6781b312d70/pandas/tests/groupby/test_transform.py#L555

The reason this is failing is because I replaced `output = {}` in the original `shift` code with `output = collections.OrderedDict()` in the newly shared function. I'm not entirely clear on why that would impact the test case, but it seems like a more explicit code path regardless. 

I believe the test case is wrong anyway and the failing line should either have a `.sort_index` call or be placed before the `expected = expected.sort_index(axis=1)` call a few lines ahead of it, but wanted to get your input and make sure I am not misreading that
> The reason this is failing is because I replaced output = {} in the original shift code with output = collections.OrderedDict() in the newly shared function. I'm not entirely clear on why that would impact the test case, but it seems like a more explicit code path regardless.

its prob happenstance that this worked before, IOW it happened to be sorted already. go ahead and add what is needed to make it pass

(and FYI make this an ``expected = ``), rather than code in ``assert_frame_equal`` block


To clarify my point about having a separate module for `_get_cythonized_result` I don't think that method as a stand-alone needs it's own module, but thinking long term and going back to the discussion in #19481 a class to more effectively dispatch all of the functions (agg and transform included) may make sense as a sub-module of `groupby.py` and `_get_cythonized_result` could potentially evolve into that
@WillAyd ahh perfect - yes absolutely in favor of s class dispatch mechanism
and should be in its own cython/python modules as needed
ok with merging this (small fix ups) then attacking that (any/all can be in current or new framework)
Hello @WillAyd! Thanks for updating the PR.







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

##### Comment last updated on February 24, 2018 at 16:41 Hours UTC
ping when switch to concat and green
@jreback all green, though it looks like AppVeyor was never triggered
One last comment, otherwise looks good to me.
Travis failure on latest commit looks unrelated (something with pyarrow?)
thanks @WillAyd very nice as always!