BUG: groupby.shift returns different columns when fill_value is specified
State: closed

- [x] closes #41556
- [x] closes #26615 
- [x] tests added / passed
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [x] whatsnew entry

This fixes the minimal reproducing examples from the original bug report #41556. 

**With master**, specifying `fill_value` causes the index columns to be returned and to include the `fill_value` in the grouping columns. 

```
In [4]: import pandas as pd
   ...: 
   ...: df = pd.DataFrame({'a': [2, 1, 2, 1], 'b': ['x', 'x', 'y', 'y']})
   ...: 
   ...: df.groupby('a').shift(1)
Out[4]:
     b
0  NaN
1  NaN
2    x
3    x
In [5]: df.groupby('a').shift(1, fill_value='fill')
Out[5]:
      a     b
0  fill  fill
1  fill  fill
2     2     x
3     1     x
```

**With this PR**, only the value columns are returned in both cases, and the fill is applied correctly.

```
In [1]: import pandas as pd
   ...: 
   ...: # On master, using fill_value causes the index columns to be returned
   ...: df = pd.DataFrame({'a': [2, 1, 2, 1], 'b': ['x', 'x', 'y', 'y']})
   ...: 
   ...: df.groupby('a').shift(1)
Out[1]:
     b
0  NaN
1  NaN
2    x
3    x
In [2]: df.groupby('a').shift(1, fill_value='fill')
Out[2]:
      b
0  fill
1  fill
2     x
3     x
```

On master, if `fill_value=None` then `_get_cythonized_result` was used. But if `fill_value` was specified, then `self.apply` was used, because `_get_cythonized_result` couldn't take the `fill_value`. I've updated `_get_cythonized_result` so it can handle the `fill_value` itself. This means `Groupby.shift` follows the same code path and returns the same structure of output, whether or not `fill_value` is specified. 

**Test**
The bug reported in the original issue should have been caught by an existing test, except that the existing test extracts the values columns ( `[['Z']]` ), before doing its comparison to the expected output. This was hiding the fact that the structure of the result depended on `fill_value` being specified. 

https://github.com/pandas-dev/pandas/blob/1.2.x/pandas/tests/groupby/test_groupby_shift_diff.py#L52

Removing the extraction of the data column ( `[['Z']]` ) means we now have a test that catches the bug reported in the original issue. (The test fails on master, but passes on this PR). 


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

Comments:
Thanks for the PR. I _think_ that dispatching to `self.apply` here is going to cause a rather large performance regression though - can check the benchmarks to see?

We likely need to patch elsewhere in the groupby code to make ensure that the correct structure is returned
> Thanks for the PR. I _think_ that dispatching to `self.apply` here is going to cause a rather large performance regression though - can check the benchmarks to see?
> 
> We likely need to patch elsewhere in the groupby code to make ensure that the correct structure is returned


@WillAyd, I think there is a misunderstanding here. I have removed the dispatch to `self.apply` and switched it to `_get_cynthonized_result`. I haven't checked but assume performance is improved. 

Also, the structure of the result no longer depends on whether you specify `fill_value` or not. 

I've updated the original PR description to make the changes clearer. 
> @WillAyd, I think there is a misunderstanding here. I have removed the dispatch to `self.apply` and switched it to `_get_cynthonized_result`. I haven't checked but would assume performance would improve.

does this close #26615
> does this close #26615

Yes, it does, @simonjayhawkins .  Using the example from #26615, they now run at same speed even when `fill_value=0` and preserve the dtype of uint8. 

```
In [1]: import numpy as np
   ...: import pandas as pd
   ...: 
   ...: SIZE_MULT = 5
   ...: data = np.random.randint(0, 255, size=10**SIZE_MULT, dtype='uint8')
   ...: index = pd.MultiIndex.from_product(
   ...:             [list(range(10**(SIZE_MULT-1))), list('ABCDEFGHIJ')],
   ...:             names = ['d', 'l'])        
   ...: test = pd.DataFrame(data, index, columns = ['data'])
   ...: print(test.dtypes['data'])
   ...:
uint8

In [2]: %%time
   ...: shifted = test.groupby(axis=0, level=[0]).shift(2)
   ...: print(shifted.dtypes['data'])
   ...:
   ...:
float64
Wall time: 15.6 ms

In [3]: %%time
   ...: shifted = test.groupby(axis=0, level=[0]).shift(2, fill_value = 0)
   ...: print(shifted.dtypes['data'])
   ...:
   ...:
uint8
Wall time: 10.9 ms
```
@WillAyd Is there anything else we need to have this merged in? 
> needs tests that reproduce the original issues

This PR includes a modification to an existing test which ensures the test recreates the original issue. The modified test fails on master because of the original issue (see below). The test passes with the code changes in this PR. 

Previously, the test extracted the value column ( `[['Z']]` ) from the `result`, ignoring the fact that the `result` contained additional columns that should have caused an error. This is fixed now. Fixing the existing test seemed more sensible than adding a new one. 

[err1429.txt](https://github.com/pandas-dev/pandas/files/6648622/err1429.txt)
```
================================== FAILURES ===================================
______________________ test_group_shift_with_fill_value _______________________

    def test_group_shift_with_fill_value():
        # GH #24128
        n_rows = 24
        df = DataFrame(
            [(i % 12, i % 3, i) for i in range(n_rows)],
            dtype=float,
            columns=["A", "B", "Z"],
            index=None,
        )
        g = df.groupby(["A", "B"])
    
        expected = DataFrame(
            [(i + 12 if i < n_rows - 12 else 0) for i in range(n_rows)],
            dtype=float,
            columns=["Z"],
            index=None,
        )
        result = g.shift(-1, fill_value=0)
    
>       tm.assert_frame_equal(result, expected)
E       AssertionError: DataFrame are different
E       
E       DataFrame shape mismatch
E       [left]:  (24, 3)
E       [right]: (24, 1)

test_groupby_shift_diff.py:60: AssertionError
```

> release note needs to be update to 1.4

Done. 

> needs review

Ready for review, @jreback 

@jreback can you take another look at this? I think it is ready to merge. 
Hello @smithto1! 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 2021-07-28 15:12:50 UTC
> looks good. can you make sure we have an asv for this case (or add one) and show the results. ping on green.
> 
> I think testing is ok, though if you can look in the original issue and make sure we have a replicated test.

@jreback please have another look, I think this is good to go. 

With this PR, the testing now replicates the original issue. I've looked into it. 

asv is added. Below, you can see that on master, using `fill_value` is slower. But on this PR branch, using `fill_value` runs in same time. 

**master**
```
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-pyc__anaconda3_envs_pandas-dev_python.exe
[ 25.00%] ··· groupby.Shift.time_defaults                              1.89±0ms
[ 50.00%] ··· groupby.Shift.time_fill_value                            3.47±0ms
```

**PR**
```
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-pyc__anaconda3_envs_pandas-dev_python.exe
[ 25.00%] ··· groupby.Shift.time_defaults                              1.33±0ms
[ 50.00%] ··· groupby.Shift.time_fill_value                            1.29±0ms
```
@smithto1 - friendly ping for resolving conflict and whatsnew request.
> @smithto1 - friendly ping for resolving conflict and whatsnew request.

That is addressed. I think this is ready to merge. 

@rhshadrach @jreback @WillAyd 
thanks @smithto1 for the patch and the patience! keep em coming!