Repository: pandas-dev/pandas
Pull request: Fix 'observed' kwarg not doing anything on SeriesGroupBy
- [x] closes #24880 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
`observed` keyword for SeriesGroupBy Ignored
#### Code Sample, a copy-pastable example if possible

```python
import pandas as pd                                                                                                                                                                                                                    
df = pd.DataFrame({'a': ['x','x','y'], 'b': ['a','b','a'], 'c': [7,8,9]})                                                                                                                                                                    
df['a'] = df['a'].astype('category')
df['b'] = df['b'].astype('category')
result1 = df.groupby(['a','b']).c.agg('sum')
result2 = df.groupby(['a','b']).agg('sum')
```

#### Problem description

The calculated `result1` and `result2` DataFrames are different.
Result1:
```
a  b
x  a    7
   b    8
y  a    9
Name: c, dtype: int64
```
Result2
```
       c
a b     
x a  7.0
  b  8.0
y a  9.0
  b  NaN
```

#### Expected Output

I expect that both results have 4 rows, as the `observed` option is `False` by default.

#### Output of ``pd.show_versions()``

<details>
INSTALLED VERSIONS
------------------
commit: None
python: 3.6.6.final.0
python-bits: 64
OS: Darwin
OS-release: 17.7.0
machine: x86_64
processor: i386
byteorder: little
LC_ALL: None
LANG: None
LOCALE: None.None
pandas: 0.23.4
pytest: None
pip: 18.1
setuptools: 40.6.3
Cython: None
numpy: 1.16.0
scipy: None
pyarrow: None
xarray: None
IPython: None
sphinx: None
patsy: None
dateutil: 2.7.5
pytz: 2018.7
blosc: None
bottleneck: None
tables: None
numexpr: None
feather: None
matplotlib: None
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
lxml: None
bs4: None
html5lib: None
sqlalchemy: None
pymysql: None
psycopg2: None
jinja2: 2.10
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: None
</details>

Hints from issue discussion:
Hmm OK thanks for the report. Something must be going awry with the `observed` keyword in the former case (which is a `SeriesGroupBy`)

```python
In [20]: df.groupby(['a','b'], observed=True).c.sum()                           
Out[20]: 
a  b
x  a    7
   b    8
y  a    9
Name: c, dtype: int64

In [21]: df.groupby(['a','b'], observed=False).c.sum()                          
Out[21]: 
a  b
x  a    7
   b    8
y  a    9
Name: c, dtype: int64
```

Investigation and PRs would certainly be welcome
check this on master
we just merged a patch that affected this
Confirmed on master

Commit message: Remove too specific fixtures and adjust tests

Patch to review:
diff --git a/pandas/core/groupby/generic.py b/pandas/core/groupby/generic.py
index 2f665975f96bd..dc414a588a2ce 100644
--- a/pandas/core/groupby/generic.py
+++ b/pandas/core/groupby/generic.py
@@ -25,7 +25,6 @@
 from pandas.core.dtypes.missing import isna, notna
 
 import pandas.core.algorithms as algorithms
-from pandas.core.arrays import Categorical
 from pandas.core.base import DataError, SpecificationError
 import pandas.core.common as com
 from pandas.core.frame import DataFrame
@@ -33,7 +32,7 @@
 from pandas.core.groupby import base
 from pandas.core.groupby.groupby import (
     GroupBy, _apply_docs, _transform_template)
-from pandas.core.index import CategoricalIndex, Index, MultiIndex
+from pandas.core.index import Index, MultiIndex
 import pandas.core.indexes.base as ibase
 from pandas.core.internals import BlockManager, make_block
 from pandas.core.series import Series
@@ -834,9 +833,10 @@ def _wrap_output(self, output, index, names=None):
             return Series(output, index=index, name=name)
 
     def _wrap_aggregated_output(self, output, names=None):
-        return self._wrap_output(output=output,
-                                 index=self.grouper.result_index,
-                                 names=names)
+        result = self._wrap_output(output=output,
+                                   index=self.grouper.result_index,
+                                   names=names)
+        return self._reindex_output(result)._convert(datetime=True)
 
     def _wrap_transformed_output(self, output, names=None):
         return self._wrap_output(output=output,
@@ -856,13 +856,16 @@ def _get_index():
             return index
 
         if isinstance(values[0], dict):
-            # GH #823
+            # GH #823 #24880
             index = _get_index()
-            result = DataFrame(values, index=index).stack()
+            result = self._reindex_output(DataFrame(values, index=index))
+            # if self.observed is False,
+            # keep all-NaN rows created while re-indexing
+            result = result.stack(dropna=self.observed)
             result.name = self._selection_name
             return result
 
-        if isinstance(values[0], (Series, dict)):
+        if isinstance(values[0], Series):
             return self._concat_objects(keys, values,
                                         not_indexed_same=not_indexed_same)
         elif isinstance(values[0], DataFrame):
@@ -870,9 +873,11 @@ def _get_index():
             return self._concat_objects(keys, values,
                                         not_indexed_same=not_indexed_same)
         else:
-            # GH #6265
-            return Series(values, index=_get_index(),
-                          name=self._selection_name)
+            # GH #6265 #24880
+            result = Series(data=values,
+                            index=_get_index(),
+                            name=self._selection_name)
+            return self._reindex_output(result)
 
     def _aggregate_named(self, func, *args, **kwargs):
         result = OrderedDict()
@@ -1335,7 +1340,8 @@ def _gotitem(self, key, ndim, subset=None):
             if subset is None:
                 subset = self.obj[key]
             return SeriesGroupBy(subset, selection=key,
-                                 grouper=self.grouper)
+                                 grouper=self.grouper,
+                                 observed=self.observed)
 
         raise AssertionError("invalid ndim for _gotitem")
 
@@ -1407,69 +1413,6 @@ def _wrap_agged_blocks(self, items, blocks):
 
         return self._reindex_output(result)._convert(datetime=True)
 
-    def _reindex_output(self, result):
-        """
-        If we have categorical groupers, then we want to make sure that
-        we have a fully reindex-output to the levels. These may have not
-        participated in the groupings (e.g. may have all been
-        nan groups);
-
-        This can re-expand the output space
-        """
-
-        # we need to re-expand the output space to accomodate all values
-        # whether observed or not in the cartesian product of our groupes
-        groupings = self.grouper.groupings
-        if groupings is None:
-            return result
-        elif len(groupings) == 1:
-            return result
-
-        # if we only care about the observed values
-        # we are done
-        elif self.observed:
-            return result
-
-        # reindexing only applies to a Categorical grouper
-        elif not any(isinstance(ping.grouper, (Categorical, CategoricalIndex))
-                     for ping in groupings):
-            return result
-
-        levels_list = [ping.group_index for ping in groupings]
-        index, _ = MultiIndex.from_product(
-            levels_list, names=self.grouper.names).sortlevel()
-
-        if self.as_index:
-            d = {self.obj._get_axis_name(self.axis): index, 'copy': False}
-            return result.reindex(**d)
-
-        # GH 13204
-        # Here, the categorical in-axis groupers, which need to be fully
-        # expanded, are columns in `result`. An idea is to do:
-        # result = result.set_index(self.grouper.names)
-        #                .reindex(index).reset_index()
-        # but special care has to be taken because of possible not-in-axis
-        # groupers.
-        # So, we manually select and drop the in-axis grouper columns,
-        # reindex `result`, and then reset the in-axis grouper columns.
-
-        # Select in-axis groupers
-        in_axis_grps = ((i, ping.name) for (i, ping)
-                        in enumerate(groupings) if ping.in_axis)
-        g_nums, g_names = zip(*in_axis_grps)
-
-        result = result.drop(labels=list(g_names), axis=1)
-
-        # Set a temp index and reindex (possibly expanding)
-        result = result.set_index(self.grouper.result_index
-                                  ).reindex(index, copy=False)
-
-        # Reset in-axis grouper columns
-        # (using level numbers `g_nums` because level names may not be unique)
-        result = result.reset_index(level=g_nums)
-
-        return result.reset_index(drop=True)
-
     def _iterate_column_groupbys(self):
         for i, colname in enumerate(self._selected_obj.columns):
             yield colname, SeriesGroupBy(self._selected_obj.iloc[:, i],
diff --git a/pandas/core/groupby/groupby.py b/pandas/core/groupby/groupby.py
index 4e9e3b4963b6d..75d678ae277e1 100644
--- a/pandas/core/groupby/groupby.py
+++ b/pandas/core/groupby/groupby.py
@@ -36,13 +36,14 @@ class providing the base-class of operations.
 from pandas.api.types import (
     is_datetime64_dtype, is_integer_dtype, is_object_dtype)
 import pandas.core.algorithms as algorithms
+from pandas.core.arrays import Categorical
 from pandas.core.base import (
     DataError, GroupByError, PandasObject, SelectionMixin, SpecificationError)
 import pandas.core.common as com
 from pandas.core.frame import DataFrame
 from pandas.core.generic import NDFrame
 from pandas.core.groupby import base
-from pandas.core.index import Index, MultiIndex
+from pandas.core.index import CategoricalIndex, Index, MultiIndex
 from pandas.core.series import Series
 from pandas.core.sorting import get_group_index_sorter
 
@@ -2301,6 +2302,69 @@ def tail(self, n=5):
         mask = self._cumcount_array(ascending=False) < n
         return self._selected_obj[mask]
 
+    def _reindex_output(self, result):
+        """
+        If we have categorical groupers, then we want to make sure that
+        we have a fully reindex-output to the levels. These may have not
+        participated in the groupings (e.g. may have all been
+        nan groups);
+
+        This can re-expand the output space
+        """
+
+        # we need to re-expand the output space to accomodate all values
+        # whether observed or not in the cartesian product of our groupes
+        groupings = self.grouper.groupings
+        if groupings is None:
+            return result
+        elif len(groupings) == 1:
+            return result
+
+        # if we only care about the observed values
+        # we are done
+        elif self.observed:
+            return result
+
+        # reindexing only applies to a Categorical grouper
+        elif not any(isinstance(ping.grouper, (Categorical, CategoricalIndex))
+                     for ping in groupings):
+            return result
+
+        levels_list = [ping.group_index for ping in groupings]
+        index, _ = MultiIndex.from_product(
+            levels_list, names=self.grouper.names).sortlevel()
+
+        if self.as_index:
+            d = {self.obj._get_axis_name(self.axis): index, 'copy': False}
+            return result.reindex(**d)
+
+        # GH 13204
+        # Here, the categorical in-axis groupers, which need to be fully
+        # expanded, are columns in `result`. An idea is to do:
+        # result = result.set_index(self.grouper.names)
+        #                .reindex(index).reset_index()
+        # but special care has to be taken because of possible not-in-axis
+        # groupers.
+        # So, we manually select and drop the in-axis grouper columns,
+        # reindex `result`, and then reset the in-axis grouper columns.
+
+        # Select in-axis groupers
+        in_axis_grps = ((i, ping.name) for (i, ping)
+                        in enumerate(groupings) if ping.in_axis)
+        g_nums, g_names = zip(*in_axis_grps)
+
+        result = result.drop(labels=list(g_names), axis=1)
+
+        # Set a temp index and reindex (possibly expanding)
+        result = result.set_index(self.grouper.result_index
+                                  ).reindex(index, copy=False)
+
+        # Reset in-axis grouper columns
+        # (using level numbers `g_nums` because level names may not be unique)
+        result = result.reset_index(level=g_nums)
+
+        return result.reset_index(drop=True)
+
 
 GroupBy._add_numeric_operations()
 
diff --git a/pandas/tests/groupby/conftest.py b/pandas/tests/groupby/conftest.py
index cb4fe511651ee..3a8ca2383a091 100644
--- a/pandas/tests/groupby/conftest.py
+++ b/pandas/tests/groupby/conftest.py
@@ -76,3 +76,13 @@ def three_group():
                       'D': np.random.randn(11),
                       'E': np.random.randn(11),
                       'F': np.random.randn(11)})
+
+
+@pytest.fixture
+def df_cat():
+    df = DataFrame({'a': ['one', 'one', 'one', 'two'],
+                    'b': ['foo', 'foo', 'bar', 'foo'],
+                    'c': [1, 2, 3, 4]})
+    df['a'] = df['a'].astype('category')
+    df['b'] = df['b'].astype('category')
+    return df
diff --git a/pandas/tests/groupby/test_categorical.py b/pandas/tests/groupby/test_categorical.py
index 112f7629d735a..484dac298362f 100644
--- a/pandas/tests/groupby/test_categorical.py
+++ b/pandas/tests/groupby/test_categorical.py
@@ -1,3 +1,4 @@
+from collections import OrderedDict
 from datetime import datetime
 
 import numpy as np
@@ -963,3 +964,65 @@ def test_shift(fill_value):
                               categories=['a', 'b', 'c', 'd'], ordered=False)
     res = ct.shift(1, fill_value=fill_value)
     assert_equal(res, expected)
+
+
+@pytest.mark.parametrize('operation', ['agg', 'apply'])
+def test_groupby_series_observed_true(df_cat, operation):
+    # GH 24880
+    index = {
+        'agg': MultiIndex.from_frame(df_cat[['a', 'b']].drop_duplicates()),
+        'apply': MultiIndex.from_tuples(
+            [tuple(grp) for grp in
+             df_cat.select_dtypes('category').drop_duplicates().values],
+            names=df_cat.select_dtypes('category'))
+    }[operation]
+
+    expected = pd.Series(data=[3, 3, 4], index=index, name='c')
+    grouped = df_cat.groupby(['a', 'b'], observed=True)['c']
+    result = getattr(grouped, operation)(sum)
+    assert_series_equal(result, expected)
+
+
+@pytest.mark.parametrize('operation', ['agg', 'apply'])
+@pytest.mark.parametrize('observed', [False, None])
+def test_groupby_series_observed_false_or_none(df_cat, observed, operation):
+    # GH 24880
+    index, _ = MultiIndex.from_product(
+        iterables=(CategoricalIndex(data=d)
+                   for d in np.apply_along_axis(
+            np.unique, 1, df_cat.select_dtypes('category').T.values)),
+        names=df_cat.select_dtypes('category').columns).sortlevel()
+
+    expected = pd.Series(data=[3, 3, np.nan, 4], index=index, name='c')
+    grouped = df_cat.groupby(['a', 'b'], observed=observed)['c']
+    result = getattr(grouped, operation)(sum)
+    assert_series_equal(result, expected)
+
+
+@pytest.mark.parametrize("observed, data", [
+    (True, [1, 2, 3, 3, 4, 4]),
+    (False, [3, 3, 1, 2, np.nan, np.nan, 4.0, 4.0]),
+    (None, [3, 3, 1, 2, np.nan, np.nan, 4.0, 4.0])])
+def test_groupby_series_observed_apply_dict(df_cat, observed, data):
+    # GH 24880
+    index_names = df_cat.select_dtypes(
+        'category').columns.values.tolist() + [None]
+    index = {
+        True: MultiIndex.from_tuples(
+            [tuple(list(grp) + [p])
+             for grp in df_cat.select_dtypes(
+                'category').drop_duplicates().values
+             for p in ('min', 'max')],
+            names=index_names),
+        False: MultiIndex.from_product(
+            [CategoricalIndex(data=d)
+             for d in np.apply_along_axis(
+                np.unique, 1, df_cat.select_dtypes('category').T.values)
+             ] + [Index(['min', 'max'])],
+            names=index_names)
+    }[bool(observed)]
+
+    expected = pd.Series(data=data, index=index, name='c')
+    result = df_cat.groupby(['a', 'b'], observed=observed)['c'].apply(
+        lambda x: OrderedDict([('min', x.min()), ('max', x.max())]))
+    assert_series_equal(result, expected)


Code review comments:
diff_hunk: @@ -963,3 +964,65 @@ def test_shift(fill_value):
                               categories=['a', 'b', 'c', 'd'], ordered=False)
     res = ct.shift(1, fill_value=fill_value)
     assert_equal(res, expected)
+
+
+@pytest.mark.parametrize('operation', ['agg', 'apply'])
+def test_groupby_series_observed_true(df_cat, operation):
+    # GH 24880
+    index = {
+        'agg': MultiIndex.from_frame(df_cat[['a', 'b']].drop_duplicates()),
+        'apply': MultiIndex.from_tuples(
+            [tuple(grp) for grp in
+             df_cat.select_dtypes('category').drop_duplicates().values],
+            names=df_cat.select_dtypes('category'))
+    }[operation]
+
+    expected = pd.Series(data=[3, 3, 4], index=index, name='c')
+    grouped = df_cat.groupby(['a', 'b'], observed=True)['c']
+    result = getattr(grouped, operation)(sum)
+    assert_series_equal(result, expected)
+
+
+@pytest.mark.parametrize('operation', ['agg', 'apply'])
+@pytest.mark.parametrize('observed', [False, None])
+def test_groupby_series_observed_false_or_none(df_cat, observed, operation):
+    # GH 24880
+    index, _ = MultiIndex.from_product(
path: pandas/tests/groupby/test_categorical.py
text: @user1:
Similar comment around just constructing from literals

diff_hunk: @@ -963,3 +964,65 @@ def test_shift(fill_value):
                               categories=['a', 'b', 'c', 'd'], ordered=False)
     res = ct.shift(1, fill_value=fill_value)
     assert_equal(res, expected)
+
+
+@pytest.mark.parametrize('operation', ['agg', 'apply'])
+def test_groupby_series_observed_true(df_cat, operation):
+    # GH 24880
+    index = {
+        'agg': MultiIndex.from_frame(df_cat[['a', 'b']].drop_duplicates()),
path: pandas/tests/groupby/test_categorical.py
text: @user1:
I think it would be better to just use literal values here instead of deriving via these expressions; it would be easier to read and could be passed in via parametrization instead of using this local dict