Repository: pandas-dev/pandas
Pull request: Bug in Series.groupby would raise ValueError when grouping by PeriodIndex level
- [ ] closes #34010
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Problem statement:
BUG: ValueError: Given date string not likely a datetime when grouping by PeriodIndex level
- [x] I have checked that this issue has not already been reported.
- [x] I have confirmed this bug exists on the latest version of pandas.
- [x] (optional) I have confirmed this bug exists on the master branch of pandas.

---

#### Code Sample, a copy-pastable example

```pytb
In [10]: t = pd.Series([1], index=pd.PeriodIndex(['2000'], name="A", freq="D"))

In [11]: t.groupby(level="A").size()
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/indexes/period.py in get_loc(self, key, method, tolerance)
    593         try:
--> 594             return self._engine.get_loc(key)
    595         except KeyError:

pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc()

pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc()

pandas/_libs/index_class_helper.pxi in pandas._libs.index.Int64Engine._check_type()

KeyError: 'A'

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
<ipython-input-11-ca2eac2f77ff> in <module>
----> 1 t.groupby("A").size()

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/series.py in groupby(self, by, axis, level, as_index, sort, group_keys, squeeze, observed)
   1685             group_keys=group_keys,
   1686             squeeze=squeeze,
-> 1687             observed=observed,
   1688         )
   1689

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/groupby/groupby.py in __init__(self, obj, keys, axis, level, grouper, exclusions, selection, as_index, sort, group_keys, squeeze, observed, mutated)
    407                 sort=sort,
    408                 observed=observed,
--> 409                 mutated=self.mutated,
    410             )
    411

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/groupby/grouper.py in get_grouper(obj, key, axis, level, sort, observed, mutated, validate)
    588
    589         elif is_in_axis(gpr):  # df.groupby('name')
--> 590             if gpr in obj:
    591                 if validate:
    592                     obj._check_label_or_level_ambiguity(gpr, axis=axis)

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/generic.py in __contains__(self, key)
   1848     def __contains__(self, key) -> bool_t:
   1849         """True if the key is in the info axis"""
-> 1850         return key in self._info_axis
   1851
   1852     @property

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/indexes/period.py in __contains__(self, key)
    384         else:
    385             try:
--> 386                 self.get_loc(key)
    387                 return True
    388             except (TypeError, KeyError):

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/indexes/period.py in get_loc(self, key, method, tolerance)
    598
    599             try:
--> 600                 asdt, parsed, reso = parse_time_string(key, self.freq)
    601                 key = asdt
    602             except TypeError:

pandas/_libs/tslibs/parsing.pyx in pandas._libs.tslibs.parsing.parse_time_string()

pandas/_libs/tslibs/parsing.pyx in pandas._libs.tslibs.parsing.parse_datetime_string_with_reso()

ValueError: Given date string not likely a datetime.
```

#### Problem description

With a regular Index, we see the following

```python

In [13]: s = pd.Series([1], index=pd.Index(['a'], name='A'))

In [14]: s.groupby(level="A").size()
Out[14]:
A
a    1
dtype: int64
```

This seems to only affect PeriodIndex. DatetimeIndex works fine.

#### Expected Output

```python
A
2000-01-01    1
Freq: D, dtype: int64
```

Hints from issue discussion:
Tagged as a regression, since this works fine in 0.25, but is broken in 1.0.3
> Tagged as a regression, since this works fine in 0.25, but is broken in 1.0.3

regression from #27909 (i.e. 1.0.0)

9cb5de04bc61f23047eb7f34bef2bb14ef58da8e is the first bad commit
commit 9cb5de04bc61f23047eb7f34bef2bb14ef58da8e
Author: jbrockmendel <jbrockmendel@gmail.com>
Date:   Tue Sep 3 04:39:20 2019 -0700

    CLN: Catch more specific exceptions in groupby (#27909)


cc @jbrockmendel

Commit message: add test for 34010

Patch to review:
diff --git a/pandas/core/groupby/grouper.py b/pandas/core/groupby/grouper.py
index f84ca6c05f40f..6c7668d85a1c1 100644
--- a/pandas/core/groupby/grouper.py
+++ b/pandas/core/groupby/grouper.py
@@ -607,11 +607,9 @@ def is_in_axis(key) -> bool:
 
     # if the grouper is obj[name]
     def is_in_obj(gpr) -> bool:
-        if not hasattr(gpr, "name"):
-            return False
         try:
-            return gpr is obj[gpr.name]
-        except (KeyError, IndexError):
+            return id(gpr) == id(obj[gpr.name])
+        except Exception:
             return False
 
     for i, (gpr, level) in enumerate(zip(keys, levels)):
diff --git a/pandas/tests/groupby/test_function.py b/pandas/tests/groupby/test_function.py
index 93dd1bf23c308..195bea7b74555 100644
--- a/pandas/tests/groupby/test_function.py
+++ b/pandas/tests/groupby/test_function.py
@@ -1307,6 +1307,14 @@ def test_size_groupby_all_null():
     tm.assert_series_equal(result, expected)
 
 
+def test_size_period_index():
+    # https://github.com/pandas-dev/pandas/issues/34010
+    ser = Series([1], index=pd.PeriodIndex(["2000"], name="A", freq="D"))
+    grp = ser.groupby(level="A")
+    result = grp.size()
+    tm.assert_series_equal(result, ser)
+
+
 # quantile
 # --------------------------------
 


Code review comments:
diff_hunk: @@ -607,11 +607,9 @@ def is_in_axis(key) -> bool:
 
     # if the grouper is obj[name]
     def is_in_obj(gpr) -> bool:
-        if not hasattr(gpr, "name"):
-            return False
         try:
-            return gpr is obj[gpr.name]
-        except (KeyError, IndexError):
+            return id(gpr) == id(obj[gpr.name])
+        except Exception:
path: pandas/core/groupby/grouper.py
text: @user3:
Probably don't want to do this. I think the fix should rather be in `Series.__contains__`.

```python

In [11]: s = pd.Series([1, 2, 3], index=pd.period_range('2000', periods=3, name='A'))

In [12]: "A" in s
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-12-6445c229fb58> in <module>
----> 1 "A" in s

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/generic.py in __contains__(self, key)
   1737     def __contains__(self, key) -> bool_t:
   1738         """True if the key is in the info axis"""
-> 1739         return key in self._info_axis
   1740
   1741     @user1

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/util/_decorators.py in wrapper(*args, **kwargs)
    353         @user2(func)
    354         def wrapper(*args, **kwargs) -> Callable:
--> 355             return func(*args, **kwargs)
    356
    357         # collecting docstring and docstring templates

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/indexes/period.py in __contains__(self, key)
    332             hash(key)
    333             try:
--> 334                 self.get_loc(key)
    335                 return True
    336             except KeyError:

~/Envs/dask-dev/lib/python3.7/site-packages/pandas/core/indexes/period.py in get_loc(self, key, method, tolerance)
    501
    502             try:
--> 503                 asdt, reso = parse_time_string(key, self.freq)
    504             except DateParseError as err:
    505                 # A string with invalid format

pandas/_libs/tslibs/parsing.pyx in pandas._libs.tslibs.parsing.parse_time_string()

pandas/_libs/tslibs/parsing.pyx in pandas._libs.tslibs.parsing.parse_datetime_string_with_reso()

ValueError: Given date string not likely a datetime.
```

should raise a KeyError?


@user3:
Or perhaps the fix would be in `Series.__getitem__`.

@author:
the plan is to backport this, so hoping to keep changes minimal. (if not reverting)

added a TODO as suggested https://github.com/pandas-dev/pandas/pull/34049#discussion_r421570058 for now

@user4:
make sense.  longer-term i think Tom's approach is optimal

diff_hunk: @@ -607,11 +607,9 @@ def is_in_axis(key) -> bool:
 
     # if the grouper is obj[name]
     def is_in_obj(gpr) -> bool:
-        if not hasattr(gpr, "name"):
-            return False
         try:
-            return gpr is obj[gpr.name]
-        except (KeyError, IndexError):
+            return id(gpr) == id(obj[gpr.name])
path: pandas/core/groupby/grouper.py
text: @user1:
how is this different?

@author:
just reverting the change from the original PR as opening gambit. plan to backport so either the old code or minimal changes works for me.

diff_hunk: @@ -607,11 +607,9 @@ def is_in_axis(key) -> bool:
 
     # if the grouper is obj[name]
     def is_in_obj(gpr) -> bool:
-        if not hasattr(gpr, "name"):
-            return False
         try:
-            return gpr is obj[gpr.name]
-        except (KeyError, IndexError):
+            return id(gpr) == id(obj[gpr.name])
+        except Exception:
path: pandas/core/groupby/grouper.py
text: @user1:
the test this adds raises a ValueError here.  instead of catching Exception, lets add ValueError and a comment about how it gets raised