BUG : ValueError in case on NaN value in groupby columns

[cgangwar11]

Fixes GH24740

• closes Exception when grouping on a modified category #24740
• closes BUG: Wrong grouping of categoricals when observed=True #21151
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Before

In [5]: ds.groupby("b").groups                                                                                   
Out[5]: {'b': Int64Index([1], dtype='int64'), 'c': Int64Index([2], dtype='int64')}
In [6]: ds.groupby("b", observed=True).groups                                                                    
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-d76af2ea5948> in <module>
----> 1 ds.groupby("b", observed=True).groups

~/Softwares/pandas/pandas/core/groupby/groupby.py in groups(self)
    388         """
    389         self._assure_grouper()
--> 390         return self.grouper.groups
    391 

After

In [5]: ds.groupby('b',observed=True).groups                                                                     
Out[5]: {'b': Int64Index([1], dtype='int64'), 'c': Int64Index([2], dtype='int64')}

Better Example

In [6]:  df = pd.DataFrame({'cat': pd.Categorical(['a', 'c', 'a'], 
   ...:                        categories=['a', 'b', 'd', 'e', 'f']), 
   ...:                        'vals': [1, 2, 3]})                                                               

In [7]: df                                                                                                       
Out[7]: 
   cat  vals
0    a     1
1  NaN     2
2    a     3

In [8]: df.groupby('cat').groups                                                                                 
Out[8]: 
{'a': Int64Index([0, 2], dtype='int64'),
 'b': Int64Index([], dtype='int64'),
 'd': Int64Index([], dtype='int64'),
 'e': Int64Index([], dtype='int64'),
 'f': Int64Index([], dtype='int64')}

In [9]: df.groupby('cat',observed=True).groups                                                                   
Out[9]: {'a': Int64Index([0, 2], dtype='int64')}

[codecov]

Codecov Report

Merging #24850 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24850      +/-   ##
==========================================
+ Coverage   92.39%   92.39%   +<.01%     
==========================================
  Files         166      166              
  Lines       52391    52392       +1     
==========================================
+ Hits        48407    48408       +1     
  Misses       3984     3984

Flag	Coverage Δ
#multiple	90.81% <100%> (ø)	⬆️
#single	42.89% <0%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/groupby/grouper.py	98.16% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 518b237...ee1c9af. Read the comment docs.

[codecov]

Codecov Report

Merging #24850 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24850      +/-   ##
==========================================
- Coverage   92.39%   92.39%   -0.01%     
==========================================
  Files         166      166              
  Lines       52391    52411      +20     
==========================================
+ Hits        48407    48423      +16     
- Misses       3984     3988       +4

Flag	Coverage Δ
#multiple	90.81% <100%> (-0.01%)	⬇️
#single	42.88% <0%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/groupby/grouper.py	98.16% <100%> (ø)	⬆️
pandas/core/tools/datetimes.py	85.21% <0%> (-0.54%)	⬇️
pandas/io/parsers.py	95.34% <0%> (-0.06%)	⬇️
pandas/core/reshape/util.py	100% <0%> (ø)	⬆️
pandas/core/indexes/base.py	96.32% <0%> (ø)	⬆️
pandas/core/arrays/datetimes.py	97.79% <0%> (+0.01%)	⬆️
pandas/core/arrays/timedeltas.py	88.3% <0%> (+0.09%)	⬆️
pandas/core/computation/expr.py	88.68% <0%> (+0.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 518b237...8d5b896. Read the comment docs.

[WillAyd]

You don't need observed as a keyword argument here; can just pass the literal value in the groupby call

[cgangwar11]

Also Fixes #21151

[TomAugspurger]

pandas.core.groupby.groups isn't a method. How about just "Bug in groupby causing a ValueError if ..."

[TomAugspurger]

Could you make this pd.Categorical(['a', np.nan, 'a'], categories=['a', 'b, 'd', 'e', 'f'])? It wasn't immediately obvious that ''c'` would end up as NA.

[jreback]

will look in a bit

[jreback]

use the observed fixture here to test both cases

[jreback]

if u claim this fixes other issues then pls explicitly test them and list in the whatsnew; these must be tested with the observed fixture

[TomAugspurger]

This does test #21151.

[jreback]

there is no indication it tests any other issue

[TomAugspurger]

@cgangwar11 can you add GH-21151 as a comment in your regression test and add it to the closes in the original post on the PR?

@jreback what's the motivation for using the observed keyword? This is a regression test specifically for observed=True. I feel like parametrizing it makes the test more complicated that necessary.

[jreback]

well these are very tricky tests and both cases must be tested

[TomAugspurger]

Fair enough. I would assume that `observed=False` is already tested, but probably best to have them both in one place.

…

On Mon, Jan 21, 2019 at 4:03 PM Jeff Reback ***@***.***> wrote: well these are very tricky tests and both cases must be tested — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24850 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIgJFGjg8tVvZfmy8cfrVBoO71oFZks5vFjlJgaJpZM4aJ2jU> .

[jreback]

@cgangwar11 pls update

[jreback]

small whatsnew comment

[jreback]

you can make these the same whatsnew entry. also this is specifically if grouping on a Categorical, pls say that.

[cgangwar11]

@jreback Changes done as requested !!!

[jreback]

thanks @cgangwar11