BUG: DataFrameGroupBy.value_counts() fails if as_index=False and there are duplicate column labels
State: closed

- [x] closes #44992
- [x] tests added / passed
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit) for how to run them
- [x] whatsnew entry

DataFrameGroupBy.value_counts() now succedes if there are duplicate column labels. In addition, it fails if `as_index=False` and column labels clash with the results column, either "count" or "proportion".

Labels:
name: Groupby

Comments:
> pls don't reinvent the wheel

I don't know what you mean. I haven't reinvented anything. I just added private methods with an additional parameter as was suggested in https://github.com/pandas-dev/pandas/pull/44755#issuecomment-986258740. No new code at all and no change to the public API. 
Otherwise, please tell me how you would suggest that this is done.
> > pls don't reinvent the wheel
> 
> I don't know what you mean. I haven't reinvented anything. I just added private methods with an additional parameter as was suggested in [#44755 (comment)](https://github.com/pandas-dev/pandas/pull/44755#issuecomment-986258740). No new code at all and no change to the public API. Otherwise, please tell me how you would suggest that this is done.

adding private methods just explodes things. you can simply use concat internally (e.g. concat the index to the frame itself).
> > > pls don't reinvent the wheel
> > 
> > 
> > I don't know what you mean. I haven't reinvented anything. I just added private methods with an additional parameter as was suggested in [#44755 (comment)](https://github.com/pandas-dev/pandas/pull/44755#issuecomment-986258740). No new code at all and no change to the public API. Otherwise, please tell me how you would suggest that this is done.
> 
> adding private methods just explodes things. you can simply use concat internally (e.g. concat the index to the frame itself).

I did not want  to use private methods, nor to reinvent the wheel! 
As you know, I wanted to add `allow_duplicates` to `reset_index()`. That would make this a one liner.

So now this is basically a reinvented `reset_index()`.
BTW I could not use `Series.to_frame()` because:
- It throws away duplicate level names
- MultiIndex has thrown away some dtypes, so I need to set each column

I could have used `concat`, but then I would have had to add the name and the index and then remove the index at the end. `insert` was simpler and neater.

Perhaps this case illuminates the discussion regarding column label duplicates. Here, the caller wants fine control over the duplicate creation policy, irrespective of any overall setting in the dataframe flags.
@jreback 
> concat the index to the frame itself

> can you not just concat?

- concat does not take an Index argument.
- Index.to_frame does not handle repeated  labels.
- MultiIndex does not handle bool dtype.
- ...

So I think that my way is optimal, but I am always open to suggestions. Perhaps fixing MultiIndex.to_frame as a separate PR would be an option. But I would still need to go through the columns fixing the dtypes (as reset_index does)
> MultiIndex does not handle bool dtype.

#45061 introduces Index[bool].  Would that get us within sight of a solution?
> #45061 introduces Index[bool]. Would that get us within sight of a solution?

That would help by reducing many of the calls to `lib.maybe_convert_objects` scattered around the system. It would remove a few lines from this PR. 

IMO the "correct" solution to this PR is my #44755, whch currently seems irretrievably blocked due to the `allow_duplicates` parameter. Then it would be just one line: `result.reset_index(allow_duplicates=True)`

@jreback demands using concat instead of my code here. But I explained above why that is not working. Fixing MultiIndex.to_frame (#45245) would help, and your #45061 would simplify that slightly.

I am inclined to launch a PR for #45245 but omitting the bool fix, which would go away once #45061 was merged. But I will need to add the dreaded `allow_duplicates` parameter to `MultiIndex.to_frame`...
Looking at this, I don't see any immediately obvious way to make this simpler than what @johnzangwill has done.

IIUC this can be simplified if/when the PRs mentioned [here](https://github.com/pandas-dev/pandas/pull/45160#issuecomment-1007947557) are merged.  If we are optimistic about those going in soon-ish, then this can go on the backburner until then (and still go in for 1.5).  Otherwise, we should merge with this approach and trust @johnzangwill to follow-up with simplifications as they become feasible.

> If we are optimistic about those going in soon-ish

@jbrockmendel, I am not optimistic about my #45160 getting merged soon! @jreback strongly disagrees with my approach, which follows suggestions from other reviewers. What I would like would be for the reviewers to sort this out between them!
It could also be the case that #45061 together with my new #45318 would enable this to be completely vectorized using concat. But the correct place would be within reset_index, so it would still need #45160.
thanks @johnzangwill