BUG: aggregations were getting overwritten if they had the same name
State: closed

- [x] closes #30880 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

xref #30092 


Labels:
name: Groupby
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
nice, thanks for the PR @MarcoGorelli 

I think this PR deserves a new issue other than #30092 , so would suggest to `xref` it.
Hello @MarcoGorelli! 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-07-14 18:13:33 UTC
@jreback @WillAyd thanks for your reviews, have updated accordingly
Hi @WillAyd - sorry to chase you up, just wanted to ask if there's anything else that needs doing here or if it's alright (or indeed if it's the wrong fix altogether :) )
@WillAyd @jreback  on a second thought, is it even desirable to support this? Having identically labelled columns would bring problems later on anyway, perhaps just raising an error message if there are identically named functions would be better?

EDIT
----
Example of where this may create problems later on: suppose we have
```
    df = tm.makeTimeDataFrame()
    result = df.resample("3D").agg(
        {"A": ["ohlc", functools.partial(np.std, ddof=0), functools.partial(np.std, ddof=1)]}
    )
```
then, if we supported this, I think we'd want `result` to look like this:
```
                   A                                                  
                ohlc                                     std          
                open      high       low     close         A         A
2000-01-03 -0.296955  1.636730 -0.296955  1.636730  0.795518  0.974307
2000-01-06  0.047975  0.047975 -0.388925 -0.388925  0.218450  0.308934
2000-01-09  1.731376  1.956266  1.731376  1.956266  0.112445  0.159021
2000-01-12 -1.609325  0.943346 -1.609325 -0.238884  1.043067  1.277491
2000-01-15  1.769357  1.769357  1.769357  1.769357  0.000000       NaN
2000-01-18  0.161944  1.540337 -1.157795  1.540337  1.101594  1.349172
2000-01-21  1.927043  1.927043  1.927043  1.927043  0.000000       NaN
2000-01-24 -0.030230 -0.030230 -0.716468 -0.651387  0.309299  0.378812
2000-01-27  1.307121  1.307121  0.130263  0.130263  0.588429  0.832165
2000-01-30  0.863422  0.863422  0.565438  0.565438  0.148992  0.210706
2000-02-02 -0.239772  1.499041 -0.506017  1.499041  0.889108  1.088930
2000-02-05  0.046606  0.046606  0.046606  0.046606  0.000000       NaN
2000-02-08  1.333582  2.205428 -0.854150 -0.854150  1.286999  1.576246
2000-02-11  0.533577  0.533577  0.533577  0.533577  0.000000       NaN
```

But then, accessing
```
result.loc[:, [('A', 'std')]]
```
would give us
```
*** pandas.core.indexes.base.InvalidIndexError: Reindexing only valid with uniquely valued Index objects
```
While weird we do technically allow for duplicate column labels, so I think OK to move forward. Looks like a few outstanding items, so let us know if you get tripped up while investigating
@WillAyd Here's something that's tripping me up - if I change line 332 of `pandas/core/groupby/generic.py` so it reads
```
        if any(isinstance(x, DataFrame) for x in results.values()):
            # let higher level handle
            return results
```

then, if I have
```
>>> import functools
>>> import pandas as pd
>>> import pandas._testing as tm
>>> import numpy as np

>>> df = tm.makeTimeDataFrame().head()
>>> df
                   A         B         C         D
2000-01-03 -1.629752 -1.277942 -0.864068 -0.693202
2000-01-04  0.661794 -0.708207  1.399000 -0.174563
2000-01-05  0.222601  1.286268  1.011485  0.055014
2000-01-06  0.031587  0.253470 -0.327816  1.396794
2000-01-07 -0.502560 -0.243002  1.266337  0.055613
```

and I do

```
>>> df.resample('3D').agg(
...     {'A': ['ohlc', functools.partial(np.std, ddof=0), functools.partial(np.std, ddof=1)]}
... )
```
then, when I get to line 284 in `pandas/core/groupby/generic.py`:
```
        if isinstance(ret, dict):
            from pandas import concat

            ret = concat(ret, axis=1)
```
I have:
```
>>> ret
{OutputKey(label='ohlc', position=0):                 open      high       low     close
2000-01-03 -1.629752  0.661794 -1.629752  0.222601
2000-01-06  0.031587  0.031587 -0.502560 -0.502560, OutputKey(label='std', position=1): 2000-01-03    0.993047
2000-01-06    0.267074
Freq: 3D, Name: A, dtype: float64, OutputKey(label='std', position=2): 2000-01-03    1.216230
2000-01-06    0.377699
Freq: 3D, Name: A, dtype: float64}

>>> concat(ret, axis=1)
                ohlc                                     std          
                   0                                       1         2
                open      high       low     close         A         A
2000-01-03 -1.629752  0.661794 -1.629752  0.222601  0.993047  1.216230
2000-01-06  0.031587  0.031587 -0.502560 -0.502560  0.267074  0.377699
>>> 
```

The problem is that there is still the position number in the columns - if I change the code so it says
```
ret = concat(ret.values(), axis=1, keys=[key.label for key in ret.keys()])
```
then I get the error
```
*** TypeError: int() argument must be a string, a bytes-like object or a number, not 'slice'
```

All I could think of to get around it is:
```
            ret = concat(ret.values(), axis=1, keys=[key.position for key in keys])
            ret.columns = ret.columns.set_levels([key.label for key in keys], level=0, verify_integrity=False)
```
, which doesn't seem right.

Also, if I instead do
```
ret = concat(ret, axis=1)
ret.columns = ret.columns.droplevel(0)
```
then `pandas/tests/groupby/aggregate/test_aggregate.py::test_order_aggregate_multiple_funcs` fails, as the order of the columns changes (because of the codes in the multiindex).
Does that only happen with ohlc? Agreed the position number shouldn't be there; may something off in the groupby `_wrap_*` functions
> Does that only happen with ohlc?

@WillAyd ohlc is the only one I've come across - are there other operations that expand dimensions?
> The problem is that there is still the position number in the columns - if I change the code so it says
> ```
> ret = concat(ret.values(), axis=1, keys=[key.label for key in ret.keys()])
> ```
> then I get the error
> ```
> *** TypeError: int() argument must be a string, a bytes-like object or a number, not 'slice'
> ```

The problem here is that when we go to initialise the MultiIndex for the columns, then in `_make_concat_multiindex` from `pandas/core/reshape/concat.py` we have that `levels` is
```python
>>> levels
[Index(['ohlc', 'std', 'std'], dtype='object'),
 Index(['A', 'close', 'high', 'low', 'open'], dtype='object')]
```
and so the problem comes from
```python
>>> levels[0].get_loc('std')
slice(1, 3, None)
```
> can you also add the test exactly from here: #30092 (and add this issue to the whatsnew)

I'm not sure how it's different - #30092 doesn't raise a `SpecificationError` anymore in 1.0.3. I've added a test to make sure it doesn't break again, but do we need a whatsnew issue for that?
> > The problem is that there is still the position number in the columns - if I change the code so it says
> > ```
> > ret = concat(ret.values(), axis=1, keys=[key.label for key in ret.keys()])
> > ```
> > 
> > 
> > then I get the error
> > ```
> > *** TypeError: int() argument must be a string, a bytes-like object or a number, not 'slice'
> > ```
> 
> The problem here is that when we go to initialise the MultiIndex for the columns, then in `_make_concat_multiindex` from `pandas/core/reshape/concat.py` we have that `levels` is
> 
> ```python
> >>> levels
> [Index(['ohlc', 'std', 'std'], dtype='object'),
>  Index(['A', 'close', 'high', 'low', 'open'], dtype='object')]
> ```
> 
> and so the problem comes from
> 
> ```python
> >>> levels[0].get_loc('std')
> slice(1, 3, None)
> ```

Have raised #33654 for this, I think I need to solve that one first
@jreback @WillAyd thanks for your reviews. Now that #33654 is closed, I've been able to update this so that the "ohlc" case passes.

I haven't been able to get rid of the special-casing of the Series case, i.e. these lines:
```python
if not isinstance(output, ABCDataFrame):
    output = output.to_frame()
```

On master the following is returned
```python
DataFrame(results)
```

But in this PR, we go through `_wrap_aggregated_output`, which may return a Series if it's input is of length 1. Do you have any suggestions on how to get around this?
> can you also add the actual test from the OP

Sure, done. Now the first part of the test matches #30880 exactly, and the second part just has `ohlc` added in
Have merged master. With the most recent changes there's no need to special-case `Series`, so have removed that branch
@jreback thanks for your review - sure, done.

>  ping on green.

TravisCI is green if I click on 'details', but for some reason still shows pending here in the summary
Before this is merged, I should add a test case using pd.NamedAgg, xref #34380
> Before this is merged, I should add a test case using pd.NamedAgg, xref #34380

@jreback, have added a test which uses pd.NamedAgg, as in #34380, it's green now 

EDIT: this is no longer allowed as of #34435, so have removed that extra test
friendly ping :)
this needs to go after #34998 which substantially refactors things
OK, thanks for letting me know, will try rebasing locally to get this ready
@MarcoGorelli hmm we are going to be refactoring #34998 / delaying it, so let's see if we can get this one in. pls rebase and see if you can address above comments.
@jreback have merged master

Regarding above comments, seems the only outstanding issue is handling when we pass an empty result to `self._wrap_aggregated_output`, which I've handled by returning `self.obj._constructor_expanddim()`. If that's not the right solution, any hints would be appreciated
@TomAugspurger if you'd have a quick look.
Thanks @MarcoGorelli!
This one took me a while (I opened it in January!) so thanks for bearing with me while I worked on it!