Repository: pandas-dev/pandas
Pull request: BUG: ngroups and len(groups) do not equal when grouping with a list of Grouper and column label (GH26326)
- [x] closes #26326 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
BUG: ngroups and len(groups) do not equal when grouping with a list of Grouper and column label
#### Code

```python
m_index = pd.MultiIndex.from_product([['Mona', 'Dennis'], ['good', 'great']], names=['name', 'desc']) 
df = pd.DataFrame({'foo': [1, 2, 1, 2], 'bar': np.random.randn(4)},  index=m_index)
n1 = df.groupby([pd.Grouper(level='name'), 'foo']).ngroups
n2 = len(df.groupby([pd.Grouper(level='name'), 'foo'])) 
n3 = len(df.groupby([pd.Grouper(level='name'), 'foo']).groups) 
print(n1, n2, n3)
# 4 2 2

```
#### Problem description

I grouped a DataFrame by a list of pandas.Grouper object and column label, the length of groups or group keys should be 4, but I checked the length of GroupBy object as n2, it is 2, why? Thanks for your answer in advance.

<details>

ommit: None
python: 3.7.3.final.0
python-bits: 64
OS: Darwin
OS-release: 18.5.0
machine: x86_64
processor: i386
byteorder: little
LC_ALL: None
LANG: zh_CN.UTF-8
LOCALE: zh_CN.UTF-8

</details>

Hints from issue discussion:
`len(<groupByObject>)` returns the number of groups as does calling the length on the `groups` property as in `n3`

`ngroups` on the other hand 

```
 def ngroups(self):
        return len(self.result_index)
```

returns the `result_index` which is a `MultiIndex` object. For the above example, the `result_index` output is the following `MultiIndex`

```
MultiIndex(levels=[['Dennis', 'Mona'], [1, 2]],
           codes=[[0, 0, 1, 1], [0, 1, 0, 1]],
           names=['name', 'foo'])
```

As of **pandas 0.24.2**, the `MultiIndex` `__len__` method is implemented as

```
def __len__(self):
        return len(self.codes[0])
```

so it returns the length of the first set of codes in the `MultiIndex`. Hence, `ngroups` outputs 4.'

---------

The groups that are formed in the example's `groupby` operation are

```
{('Dennis', 1): MultiIndex(levels=[['Dennis', 'Mona'], ['good', 'great']],
            codes=[[1], [0]],
            names=['name', 'desc']),
 ('Mona', 2): MultiIndex(levels=[['Dennis', 'Mona'], ['good', 'great']],
            codes=[[1], [1]],
            names=['name', 'desc'])}
```

It appears that the correct groups are not being formed correctly when you are passing a `pandas.Grouper` object

--------

Not sure if this is appropriate behaviour though.



This is pretty tricky. I'm surprised to see a difference between the below as well:

```python
>>> df.groupby([pd.Grouper(level=0), 'foo']).grouper.groups.keys()
dict_keys([('Dennis', 1), ('Mona', 2)])

>>> df.reset_index(level=1).groupby(['name', 'foo']).grouper.groups.keys()
dict_keys([('Dennis', 1), ('Dennis', 2), ('Mona', 1), ('Mona', 2)])

>>> df.groupby([pd.Grouper(['Mona', 'Mona', 'Dennis', 'Dennis']), 'foo']).grouper.groups.keys()
dict_keys([('Dennis', 1), ('Dennis', 2), ('Mona', 1), ('Mona', 2)])
```

I think the problem starts here:

https://github.com/pandas-dev/pandas/blob/ee6b1318ff07806ead79dac8119ad9ce580afe1c/pandas/core/groupby/ops.py#L261

Where the groups are getting constructed from the individual groupings (here the first grouping is the `pd.Grouper(level='name')` and the second grouping is the Series containing the values for `foo`).

The problem is that with the former iteration only goes over the unique values (here 'Dennis' and 'Mona') which is why your length calculations are getting truncated to 2:

https://github.com/pandas-dev/pandas/blob/ee6b1318ff07806ead79dac8119ad9ce580afe1c/pandas/core/groupby/grouper.py#L348

Not sure the best resolution off the top of my head but if you'd like to investigate further and try your hand at a PR would certainly be welcome!
When a `Grouping` is created using a `Grouper` object, the resultant `Grouping` consists of a list containing one object -> The `BaseGrouper` corresponding to the `Grouper` object initially passed. In OP's example, the first `Grouper` object is passed with `level='name'` parameter and the second is simply an index `foo`.  The `DataFrameGroupBy` finally constructed has a `BaseGrouper` object with the following `Grouping` objects

```
>>> dfGroupBy = df.grouby([pd.Grouper(level='name'), 'foo'])
>>> dfGroupBy.grouper.groupings
[Grouping(name), Grouping(foo)]
```
The `Grouping(name)` object is the one generated from the `Grouper` object. The `grouper` parameter of this `Grouping(name)` object is itself a `BaseGrouper`

```
>>> dfGroupBy.grouper.groupings[0].grouper
<pandas.core.groupby.groupby.BaseGrouper>
```

Meanwhile, if the `Grouping` is constructed simply using an `index` like `foo`

```
>> dfGroupby.grouper.groupings[1].grouper
array([1, 2, 1, 2], dtype=int64)
```

The `grouper` is a simple array.

_(Is this behavior intentional?)_

----

Also as @WillAyd mentioned, 

```
to_groupby = lzip(*(ping.grouper for ping in self.groupings))
```
when working with a `BaseGrouper` returns only the indices rather than an array of appropriate values.

-----

So, a possible fix might be either to handle the special case of a `BaseGrouper` while constructing the variable `to_groupby` or updating the `_get_grouper()` code to not have a `BaseGrouper` object as a member of a `Grouping`

Adding to the above discussion,
```
>>> dfGroupby.grouper.groupings[0].grouper.groupings[0].grouper
Index(['Mona', 'Mona', 'Dennis', 'Dennis'], dtype='object', name='name')
```
Thanks for the bug confirmation, will use combination of labels to groupby instead.
As a beginner of pandas and even Python, I am trying to understand the codes and logic you mentioned above.😂

Commit message: BUG: ngroups and len(groups) do not equal when grouping with a list of Grouper and column label (GH26326)

Patch to review:
diff --git a/doc/source/whatsnew/v0.25.0.rst b/doc/source/whatsnew/v0.25.0.rst
index dacd433f112a5..de2de7e2e8985 100644
--- a/doc/source/whatsnew/v0.25.0.rst
+++ b/doc/source/whatsnew/v0.25.0.rst
@@ -406,7 +406,7 @@ Groupby/Resample/Rolling
 - Bug in :meth:`pandas.core.groupby.GroupBy.idxmax` and :meth:`pandas.core.groupby.GroupBy.idxmin` with datetime column would return incorrect dtype (:issue:`25444`, :issue:`15306`)
 - Bug in :meth:`pandas.core.groupby.GroupBy.cumsum`, :meth:`pandas.core.groupby.GroupBy.cumprod`, :meth:`pandas.core.groupby.GroupBy.cummin` and :meth:`pandas.core.groupby.GroupBy.cummax` with categorical column having absent categories, would return incorrect result or segfault (:issue:`16771`)
 - Bug in :meth:`pandas.core.groupby.GroupBy.nth` where NA values in the grouping would return incorrect results (:issue:`26011`)
-
+- Bug in :meth:`pandas.core.groupby.ops.BaseGrouper.groups` in which a :class:`BaseGrouper` object with another :class:`BaseGrouper` as part of its :class:`Groupings` would return incorrect set of groups (:issue:`26326`)
 
 Reshaping
 ^^^^^^^^^
diff --git a/pandas/core/groupby/ops.py b/pandas/core/groupby/ops.py
index 235975a9b68a0..c7f735a7d007a 100644
--- a/pandas/core/groupby/ops.py
+++ b/pandas/core/groupby/ops.py
@@ -258,7 +258,10 @@ def groups(self):
         if len(self.groupings) == 1:
             return self.groupings[0].groups
         else:
-            to_groupby = zip(*(ping.grouper for ping in self.groupings))
+            to_groupby = zip(*(ping.grouper if not isinstance(ping.grouper,
+                                                              self.__class__)
+                             else ping.grouper.groupings[0].grouper for ping
+                             in self.groupings))
             to_groupby = Index(to_groupby)
             return self.axis.groupby(to_groupby)
 
diff --git a/pandas/tests/groupby/test_groupby.py b/pandas/tests/groupby/test_groupby.py
index 354242995d3c3..a2041f93a0ee1 100644
--- a/pandas/tests/groupby/test_groupby.py
+++ b/pandas/tests/groupby/test_groupby.py
@@ -1737,3 +1737,15 @@ def test_groupby_multiindex_series_keys_len_equal_group_axis():
     expected = pd.Series([3], index=ei)
 
     assert_series_equal(result, expected)
+
+
+def test_groupby_groups_in_BaseGrouper():
+    # https://github.com/pandas-dev/pandas/issues/26326
+    m_index = pd.MultiIndex.from_product([['A', 'B'],
+                                         ['C', 'D']], names=['alpha', 'beta'])
+    df_sample = pd.DataFrame({'foo': [1, 2, 1, 2], 'bar': [1, 2, 3, 4]},
+                             index=m_index)
+    dfGBY_BaseGrouper = df_sample.groupby([pd.Grouper(level='alpha'), 'beta'])
+    dfGBY_noBaseGrouper = df_sample.groupby(['alpha', 'beta'])
+
+    assert(dfGBY_BaseGrouper.groups == dfGBY_noBaseGrouper.groups)


Code review comments:
diff_hunk: @@ -258,7 +258,10 @@ def groups(self):
         if len(self.groupings) == 1:
             return self.groupings[0].groups
         else:
-            to_groupby = zip(*(ping.grouper for ping in self.groupings))
+            to_groupby = zip(*(ping.grouper if not isinstance(ping.grouper,
+                                                              self.__class__)
+                             else ping.grouper.groupings[0].grouper for ping
path: pandas/core/groupby/ops.py
text: @user1:
Wouldn't this only work if the grouper was the first item? Would need something more generalizable

@author:
I was wondering the same. Can Groupers be nested to more than one level?

@user1:
I think this would fail if your test was `.groupby(['beta', pd.Grouper(level='alpha')])` instead of `.groupby([pd.Grouper(level='alpha'), 'beta'])`

@author:
The problem is with the construction of the `Groupings`. The `[0]` indexes the sole `BaseGrouper` created inside the grouping due to the `_get_grouper` method. The order will be maintained. I have added another assertion to check this and it passed.

diff_hunk: @@ -406,7 +406,7 @@ Groupby/Resample/Rolling
 - Bug in :meth:`pandas.core.groupby.GroupBy.idxmax` and :meth:`pandas.core.groupby.GroupBy.idxmin` with datetime column would return incorrect dtype (:issue:`25444`, :issue:`15306`)
 - Bug in :meth:`pandas.core.groupby.GroupBy.cumsum`, :meth:`pandas.core.groupby.GroupBy.cumprod`, :meth:`pandas.core.groupby.GroupBy.cummin` and :meth:`pandas.core.groupby.GroupBy.cummax` with categorical column having absent categories, would return incorrect result or segfault (:issue:`16771`)
 - Bug in :meth:`pandas.core.groupby.GroupBy.nth` where NA values in the grouping would return incorrect results (:issue:`26011`)
-
+- Bug in :meth:`pandas.core.groupby.ops.BaseGrouper.groups` in which a :class:`BaseGrouper` object with another :class:`BaseGrouper` as part of its :class:`Groupings` would return incorrect set of groups (:issue:`26326`)
path: doc/source/whatsnew/v0.25.0.rst
text: @user1:
Can you construct a message that is more user facing? These items aren't part of the API

@author:
User facing as in more user friendly?

diff_hunk: @@ -1737,3 +1737,15 @@ def test_groupby_multiindex_series_keys_len_equal_group_axis():
     expected = pd.Series([3], index=ei)
 
     assert_series_equal(result, expected)
+
+
+def test_groupby_groups_in_BaseGrouper():
+    # https://github.com/pandas-dev/pandas/issues/26326
+    m_index = pd.MultiIndex.from_product([['A', 'B'],
path: pandas/tests/groupby/test_groupby.py
text: @user1:
Use `mi` instead of `m_index`

diff_hunk: @@ -1737,3 +1737,15 @@ def test_groupby_multiindex_series_keys_len_equal_group_axis():
     expected = pd.Series([3], index=ei)
 
     assert_series_equal(result, expected)
+
+
+def test_groupby_groups_in_BaseGrouper():
+    # https://github.com/pandas-dev/pandas/issues/26326
+    m_index = pd.MultiIndex.from_product([['A', 'B'],
+                                         ['C', 'D']], names=['alpha', 'beta'])
+    df_sample = pd.DataFrame({'foo': [1, 2, 1, 2], 'bar': [1, 2, 3, 4]},
path: pandas/tests/groupby/test_groupby.py
text: @user1:
`df` instead of `df_sample`

diff_hunk: @@ -1737,3 +1737,15 @@ def test_groupby_multiindex_series_keys_len_equal_group_axis():
     expected = pd.Series([3], index=ei)
 
     assert_series_equal(result, expected)
+
+
+def test_groupby_groups_in_BaseGrouper():
+    # https://github.com/pandas-dev/pandas/issues/26326
+    m_index = pd.MultiIndex.from_product([['A', 'B'],
+                                         ['C', 'D']], names=['alpha', 'beta'])
+    df_sample = pd.DataFrame({'foo': [1, 2, 1, 2], 'bar': [1, 2, 3, 4]},
+                             index=m_index)
+    dfGBY_BaseGrouper = df_sample.groupby([pd.Grouper(level='alpha'), 'beta'])
path: pandas/tests/groupby/test_groupby.py
text: @user1:
Maybe just `grp1` and `grp2` for better readability here and line below