Repository: pandas-dev/pandas
Pull request: DEPR: DataFrame GroupBy indexing with single items DeprecationWarning


- [x] closes #23566
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
API: DataFrameGroupBy column subset selection with single list?
I wouldn't be surprised if there is already an issue about this, but couldn't directly find one.

When doing a subselection of columns on a DataFrameGroupBy object, both a plain list (so a tuple within the `__getitem__` [] brackets) as the double square brackets (a list inside the `__getitem__` [] brackets) seems to work:

```
In [6]: df = pd.DataFrame(np.random.randint(10, size=(10, 4)), columns=['a', 'b', 'c', 'd'])

In [8]: df.groupby('a').sum()
Out[8]: 
    b   c   d
a            
0   0   5   7
3  18   6  12
4  16   6   9
6  10  11  11
9   3   3   0

In [9]: df.groupby('a')['b', 'c'].sum()
Out[9]: 
    b   c
a        
0   0   5
3  18   6
4  16   6
6  10  11
9   3   3

In [10]: df.groupby('a')[['b', 'c']].sum()
Out[10]: 
    b   c
a        
0   0   5
3  18   6
4  16   6
6  10  11
9   3   3
```

Personally I find this `df.groupby('a')['b', 'c'].sum()` a bit strange, and inconsistent with how DataFrame indexing works.

Of course, on a DataFrameGroupBy you don't have the possible confusion with indexing multiple dimensions (rows, columns), but still.

cc @jreback @WillAyd

Hints from issue discussion:
You do have ambiguity with tuples though (not that anyone should do that)

```python
In [14]: df = pd.DataFrame(np.random.randint(10, size=(10, 4)), columns=['a', 'b', 'c', ('a', 'b')])

In [15]: df.groupby('c')['a', 'b'].sum()
Out[15]:
    a   b
c
0   1   7
1   6   9
2   7   7
5   9  11
6   8   6
7  10   6
8  11   8

In [16]: df.groupby('c')[('a', 'b')].sum()
Out[16]:
    a   b
c
0   1   7
1   6   9
2   7   7
5   9  11
6   8   6
7  10   6
8  11   8
```

I think both of those are incorrect. It should rather be

```python
In [19]: df.groupby('c').sum()[('a', 'b')]
Out[19]:
c
0     7
1     3
2     5
5     7
6     8
7    16
8    11
Name: (a, b), dtype: int64
```
I don't disagree here. There is a difference when selecting only one column (specifically returning a Series vs a DataFrame) but when selecting multiple columns it would be more consistent if we ALWAYS required double brackets brackets. I assume this would also yield a simpler implementation.

Maybe a conversation piece for 1.0? Would be a breaking change for sure so probably best served in a major release like that
I think the hope is for 1.0 to be backwards compatible with 0.25.x.

Do we have a chance to detect this case and throw a FutureWarning (assuming we want to change)?
Yeah, if we want, I would think it should be possible with a deprecation cycle.
this i suspect is actually very common in the wild (not using the double brackets)

but i agree we should deprecate as it is inconsistent 
Can I take a crack at this or has it already been fixed?

Also, will this be a part of the 1.0 or other milestones?
Go for it! 

Sent from my iPhone

> On Dec 24, 2019, at 8:46 PM, Josh Dimarsky <notifications@github.com> wrote:
> 
> ﻿
> Can I take a crack at this or has it already been fixed?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

Thanks will do. 
take
So this is my first time working on pandas code, and I'm a little confused here, so please bear with me. I'm also new to linking to code on GitHub.

As I understand, when an object calls `__getitem__` by using brackets, if you pass in several keys, they are implicitly converted to a tuple of one key. So `df['a','b']` is really `df[('a','b')]` under the hood.

I'm having trouble in tracing the code path to figure out where exactly the `__getitem__` on the `GroupBy` is actually implemented here:

1. `DataFrame.groupby` is called on the superclass `NDFrame` [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/generic.py#L7286)
2. This eventually creates the specific `DataFrameGroupBy` object [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/groupby/generic.py#L818)
3. Which is a subclass of [`GroupBy`](https://github.com/pandas-dev/pandas/blob/master/pandas/core/groupby/groupby.py#L1040)
4. Which is a subclass of [`_GroupBy`](https://github.com/pandas-dev/pandas/blob/master/pandas/core/groupby/groupby.py#L357)
5. Which has the mixin named `SelectionMixin`, defined [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/base.py#L134)
6. Which implements `__getitem__` [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/base.py#L220)
7. Which, if the `key` is a list or tuple, returns `self._gotitem(list(key), ndim=2)`
8. `self._gotitem` needs to be implemented by the respective subclasses, which in this case is the `DataFrameGroupBy` object, and is implemented [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/groupby/generic.py#L1575)
9. But all this does is simply create an instance of itself (`DataFrameGroupBy`) with the `key` (a list/tuple) passed as a slice to the `selection` parameter
10. The `selection` parameter is implemented in the parent `_GroupBy` object, which sets the internal `self._selection` attribute to the `key` [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/groupby/groupby.py#L378)
11. This is where I'm lost. How does this actually slice the object and only return a subset of it?

Any help here would be greatly appreciated. Thanks.

Commit message: fixed examples in docs, added test, changed warning to only tuple with len > 1

Patch to review:
diff --git a/doc/source/getting_started/comparison/comparison_with_sas.rst b/doc/source/getting_started/comparison/comparison_with_sas.rst
index 69bb700c97b15..4e284fe7b5968 100644
--- a/doc/source/getting_started/comparison/comparison_with_sas.rst
+++ b/doc/source/getting_started/comparison/comparison_with_sas.rst
@@ -629,7 +629,7 @@ for more details and examples.
 
 .. ipython:: python
 
-   tips_summed = tips.groupby(['sex', 'smoker'])['total_bill', 'tip'].sum()
+   tips_summed = tips.groupby(['sex', 'smoker'])[['total_bill', 'tip']].sum()
    tips_summed.head()
 
 
diff --git a/doc/source/getting_started/comparison/comparison_with_stata.rst b/doc/source/getting_started/comparison/comparison_with_stata.rst
index db687386329bb..fec6bae1e0330 100644
--- a/doc/source/getting_started/comparison/comparison_with_stata.rst
+++ b/doc/source/getting_started/comparison/comparison_with_stata.rst
@@ -617,7 +617,7 @@ for more details and examples.
 
 .. ipython:: python
 
-   tips_summed = tips.groupby(['sex', 'smoker'])['total_bill', 'tip'].sum()
+   tips_summed = tips.groupby(['sex', 'smoker'])[['total_bill', 'tip']].sum()
    tips_summed.head()
 
 
diff --git a/doc/source/whatsnew/v1.0.0.rst b/doc/source/whatsnew/v1.0.0.rst
index 96ea682dd3caf..43346ac534df3 100755
--- a/doc/source/whatsnew/v1.0.0.rst
+++ b/doc/source/whatsnew/v1.0.0.rst
@@ -523,6 +523,7 @@ Deprecations
   it is recommended to use ``json_normalize`` as :func:`pandas.json_normalize` instead (:issue:`27586`).
 - :meth:`DataFrame.to_stata`, :meth:`DataFrame.to_feather`, and :meth:`DataFrame.to_parquet` argument "fname" is deprecated, use "path" instead (:issue:`23574`)
 - The deprecated internal attributes ``_start``, ``_stop`` and ``_step`` of :class:`RangeIndex` now raise a ``FutureWarning`` instead of a ``DeprecationWarning`` (:issue:`26581`)
+- When selecting columns from a :class:`DataFrameGroupBy` object, passing individual items in brackets is deprecated, should pass in a list of items instead. (:issue:`23566`)
 
 .. _whatsnew_1000.prior_deprecations:
 
diff --git a/pandas/core/groupby/generic.py b/pandas/core/groupby/generic.py
index 6b110a0c80c07..23243cff127f0 100644
--- a/pandas/core/groupby/generic.py
+++ b/pandas/core/groupby/generic.py
@@ -24,6 +24,7 @@
     Union,
     cast,
 )
+import warnings
 
 import numpy as np
 
@@ -325,7 +326,7 @@ def _aggregate_multiple_funcs(self, arg):
         return DataFrame(results, columns=columns)
 
     def _wrap_series_output(
-        self, output: Mapping[base.OutputKey, Union[Series, np.ndarray]], index: Index,
+        self, output: Mapping[base.OutputKey, Union[Series, np.ndarray]], index: Index
     ) -> Union[Series, DataFrame]:
         """
         Wraps the output of a SeriesGroupBy operation into the expected result.
@@ -1574,6 +1575,17 @@ def filter(self, func, dropna=True, *args, **kwargs):
 
         return self._apply_filter(indices, dropna)
 
+    def __getitem__(self, key):
+        # per GH 23566
+        if isinstance(key, tuple) and len(key) > 1:
+            warnings.warn(
+                "Indexing with multiple keys (implicitly converted to a tuple "
+                "of keys) will be deprecated, use a list instead.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+        return super().__getitem__(key)
+
     def _gotitem(self, key, ndim: int, subset=None):
         """
         sub-classes to define
diff --git a/pandas/tests/groupby/test_grouping.py b/pandas/tests/groupby/test_grouping.py
index f2af397357e4f..88603ad30a555 100644
--- a/pandas/tests/groupby/test_grouping.py
+++ b/pandas/tests/groupby/test_grouping.py
@@ -71,14 +71,16 @@ def test_getitem_list_of_columns(self):
         )
 
         result = df.groupby("A")[["C", "D"]].mean()
-        result2 = df.groupby("A")["C", "D"].mean()
-        result3 = df.groupby("A")[df.columns[2:4]].mean()
+        result2 = df.groupby("A")[df.columns[2:4]].mean()
 
         expected = df.loc[:, ["A", "C", "D"]].groupby("A").mean()
 
         tm.assert_frame_equal(result, expected)
         tm.assert_frame_equal(result2, expected)
-        tm.assert_frame_equal(result3, expected)
+
+        # per GH 23566 this should raise a deprecation warning
+        with tm.assert_produces_warning(DeprecationWarning):
+            df.groupby("A")["C", "D"].mean()
 
     def test_getitem_numeric_column_names(self):
         # GH #13731
@@ -91,14 +93,37 @@ def test_getitem_numeric_column_names(self):
             }
         )
         result = df.groupby(0)[df.columns[1:3]].mean()
-        result2 = df.groupby(0)[2, 4].mean()
-        result3 = df.groupby(0)[[2, 4]].mean()
+        result2 = df.groupby(0)[[2, 4]].mean()
 
         expected = df.loc[:, [0, 2, 4]].groupby(0).mean()
 
         tm.assert_frame_equal(result, expected)
         tm.assert_frame_equal(result2, expected)
-        tm.assert_frame_equal(result3, expected)
+
+        # per GH 23566 this should raise a deprecation warning
+        with tm.assert_produces_warning(DeprecationWarning):
+            df.groupby(0)[2, 4].mean()
+
+    def test_getitem_single_column(self):
+        df = DataFrame(
+            {
+                "A": ["foo", "bar", "foo", "bar", "foo", "bar", "foo", "foo"],
+                "B": ["one", "one", "two", "three", "two", "two", "one", "three"],
+                "C": np.random.randn(8),
+                "D": np.random.randn(8),
+                "E": np.random.randn(8),
+            }
+        )
+
+        result = df.groupby("A")["C"].mean()
+
+        as_frame = df.loc[:, ["A", "C"]].groupby("A").mean()
+        as_series = as_frame.iloc[:, 0]
+        expected = as_series
+
+        assert isinstance(result, Series)
+        assert not isinstance(result, DataFrame)
+        tm.assert_series_equal(result, expected)
 
 
 # grouping
diff --git a/pandas/tests/groupby/test_transform.py b/pandas/tests/groupby/test_transform.py
index c46180c1d11cd..87bb1e52855c8 100644
--- a/pandas/tests/groupby/test_transform.py
+++ b/pandas/tests/groupby/test_transform.py
@@ -319,7 +319,7 @@ def test_dispatch_transform(tsframe):
 
 def test_transform_select_columns(df):
     f = lambda x: x.mean()
-    result = df.groupby("A")["C", "D"].transform(f)
+    result = df.groupby("A")[["C", "D"]].transform(f)
 
     selection = df[["C", "D"]]
     expected = selection.groupby(df["A"]).transform(f)


Code review comments:
diff_hunk: @@ -91,14 +93,37 @@ def test_getitem_numeric_column_names(self):
             }
         )
         result = df.groupby(0)[df.columns[1:3]].mean()
-        result2 = df.groupby(0)[2, 4].mean()
-        result3 = df.groupby(0)[[2, 4]].mean()
+        result2 = df.groupby(0)[[2, 4]].mean()
 
         expected = df.loc[:, [0, 2, 4]].groupby(0).mean()
 
         tm.assert_frame_equal(result, expected)
         tm.assert_frame_equal(result2, expected)
-        tm.assert_frame_equal(result3, expected)
+
+        # per GH 23566 this should raise a deprecation warning
+        with tm.assert_produces_warning(DeprecationWarning):
+            df.groupby(0)[2, 4].mean()
+
+    def test_getitem_single_column(self):
+        df = DataFrame(
+            {
+                "A": ["foo", "bar", "foo", "bar", "foo", "bar", "foo", "foo"],
+                "B": ["one", "one", "two", "three", "two", "two", "one", "three"],
+                "C": np.random.randn(8),
+                "D": np.random.randn(8),
+                "E": np.random.randn(8),
+            }
+        )
+
+        result = df.groupby("A")["C"].mean()
+
+        as_frame = df.loc[:, ["A", "C"]].groupby("A").mean()
+        as_series = as_frame.iloc[:, 0]
+        expected = as_series
+
+        assert isinstance(result, Series)
path: pandas/tests/groupby/test_grouping.py
text: @user1:
these 2 asserts are not needed as already covered by assert_series_equal

@author:
ok, changing

diff_hunk: @@ -1574,6 +1575,17 @@ def filter(self, func, dropna=True, *args, **kwargs):
 
         return self._apply_filter(indices, dropna)
 
+    def __getitem__(self, key):
+        # per GH 23566
+        if isinstance(key, tuple) and len(key) > 1:
+            warnings.warn(
+                "Indexing with multiple keys (implicitly converted to a tuple "
+                "of keys) will be deprecated, use a list instead.",
+                DeprecationWarning,
path: pandas/core/groupby/generic.py
text: @user1:
use FutureWarning

why does this have to be len > 1?

@author:
Because if it is exactly 1 then it actually becomes a GroupBySeries for which this is valid syntax.

@user1:
ok, can you add a comment to that effect

diff_hunk: @@ -91,14 +93,37 @@ def test_getitem_numeric_column_names(self):
             }
         )
         result = df.groupby(0)[df.columns[1:3]].mean()
-        result2 = df.groupby(0)[2, 4].mean()
-        result3 = df.groupby(0)[[2, 4]].mean()
+        result2 = df.groupby(0)[[2, 4]].mean()
 
         expected = df.loc[:, [0, 2, 4]].groupby(0).mean()
 
         tm.assert_frame_equal(result, expected)
         tm.assert_frame_equal(result2, expected)
-        tm.assert_frame_equal(result3, expected)
+
+        # per GH 23566 this should raise a deprecation warning
path: pandas/tests/groupby/test_grouping.py
text: @user1:
make this a separate test