BUG : ValueError in case on NaN value in groupby columns
State: closed

Fixes GH24740

- [x] closes #24740 
- [x] closes #21151
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

**Before**
```python
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**
```python
In [5]: ds.groupby('b',observed=True).groups                                                                     
Out[5]: {'b': Int64Index([1], dtype='int64'), 'c': Int64Index([2], dtype='int64')}

```

**Better Example**
```python
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')}



```


Labels:
name: Groupby
name: Categorical
description: Categorical Data Type

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=h1) Report
> Merging [#24850](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/518b237ef515a250abf5ba403563721754d39c05?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24850/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=tree)

```diff
@@            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%> (ø)` | :arrow_up: |
| #single | `42.89% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/grouper.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGVyLnB5) | `98.16% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=footer). Last update [518b237...ee1c9af](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=h1) Report
> Merging [#24850](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/518b237ef515a250abf5ba403563721754d39c05?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24850/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=tree)

```diff
@@            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%)` | :arrow_down: |
| #single | `42.88% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/grouper.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGVyLnB5) | `98.16% <100%> (ø)` | :arrow_up: |
| [pandas/core/tools/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvZGF0ZXRpbWVzLnB5) | `85.21% <0%> (-0.54%)` | :arrow_down: |
| [pandas/io/parsers.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhcnNlcnMucHk=) | `95.34% <0%> (-0.06%)` | :arrow_down: |
| [pandas/core/reshape/util.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS91dGlsLnB5) | `100% <0%> (ø)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.32% <0%> (ø)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `97.79% <0%> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `88.3% <0%> (+0.09%)` | :arrow_up: |
| [pandas/core/computation/expr.py](https://codecov.io/gh/pandas-dev/pandas/pull/24850/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vZXhwci5weQ==) | `88.68% <0%> (+0.11%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=footer). Last update [518b237...8d5b896](https://codecov.io/gh/pandas-dev/pandas/pull/24850?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Also Fixes #21151
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
This does test https://github.com/pandas-dev/pandas/issues/21151.
there is no indication it tests any other issue

@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.
well these are very tricky tests and both cases must be tested

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 <notifications@github.com>
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
> <https://github.com/pandas-dev/pandas/pull/24850#issuecomment-456210295>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIgJFGjg8tVvZfmy8cfrVBoO71oFZks5vFjlJgaJpZM4aJ2jU>
> .
>

@cgangwar11 pls update 
@jreback Changes done as requested !!!
thanks @cgangwar11