BUG: GroupBy.get_group raises ValueError when group key contains NaT

[sinhrks]

Closes #6992. Made GroupBy.get_group works even if the key contains NaT.

NOTE: One issue is that GroupBy.groups returns incorrect key in numpy 1.6. This seems to be caused by _convert_grouper uses grouper.reindex(axis).values to return value. This looks doesn't affect to main functionalities, but is there any expected result?

import pandas as pd
import numpy as np
>>> np.__version__
1.6.2

>>> df = pd.DataFrame({'values': np.random.randn(8), 
                   'dt': [np.nan, pd.Timestamp('2013-01-01'), np.nan, pd.Timestamp('2013-02-01'),
                          np.nan, pd.Timestamp('2013-02-01'), np.nan, pd.Timestamp('2013-01-01')]})
>>> grouped = df.groupby('dt')

>>> grouped.groups
{1970-01-16 48:00:00: [1, 7], 1970-01-16 24:00:00: [3, 5]}

[jreback]

I think better to add an argument to Index.groupby and DatetimeIndex.groupby, maybe exclude_missing=False as the default (and you will pass True here), then you can leave groupby alone and do the exlusions inside the index?

[sinhrks]

OK, I've changed the logic and would like to ask about keyword.

Considering #3729, I preferred to dropna as a keyword, and now it uses dropna. But looks better to use exclude_missing because pivot_table already has dropna for different purpose (to exclude index/columns from result which values are all NaN).

Is it OK to add exclude_missing to Dataframe.groupby and pivot_table as new keyword?

Also, I think default should be dropna=True based on current behavior which exclude NaT as nan.

[jreback]

you can't have nan in the int groupby nor datetime

the cython routines are very strict about types

so u don't need to check for a non existsnt type

furthermore the datetimes won't be passed directly but as a i8 dtype

[sinhrks]

I understand nan should not be included in values of such indexes, but can be passed as to_groupby argument.

Existing cython logic doesn't care about index instance, but check nan for the argument. I didn't change the core logic, and added to Nat handling condition.

[jreback]

@sinhrks haven't really looked at this, but does this solve #7227 as well?

[sinhrks]

@jreback Unfortunately No. #7227 seems to be caused by TimeGrouper, and the fix is only for normal groupby. I'll take a deeper look later.

[cpcloud]

do something like

cdef Py_ssize_t NaT = tslib.NaT
# ...
k == NaT

no need for any overhead of attribute access.

[sinhrks]

In above example to group by datetime column, labels argument for groupby_template will be DatetimeIndex, not i8. So I understand all functions created by groupby_template should have the logic to exclude NaT

index = [0 1 2 3 4 5 6 7]
labels = <class 'pandas.tseries.index.DatetimeIndex'>
[NaT, ..., 2013-01-01]
Length: 8, Freq: None, Timezone: None

[jreback]

hmm...DatetimeIndex should never be passed to a cython function, ONLY i8. can you show me where that is the case? (you can pass as object dtype, but i8 is MUCH faster so always pass that way)

[sinhrks]

@jreback The behavior seems to be caused by Grouping.
https://github.com/pydata/pandas/blob/master/pandas/core/groupby.py#L1881-1884

grouper is converted to datetime in following lines.
https://github.com/pydata/pandas/blob/master/pandas/core/groupby.py#L1831-1838

[jreback]

@sinhrks that doesn't seem a problem, its when its actually passed to cython that needs conversion (to and from), can you investigate?

[jreback]

@sinhrks can you rebase and address comments?

[sinhrks]

@jreback You mean to convert to_groupby (group key) just before passed to cython func, correct?
https://github.com/pydata/pandas/blob/master/pandas/core/index.py#L1330-1331

Conversion looks possible if to_gorupby is datetime, but it also can be an object dtype.
Thus I think cython funcs should handle object in this case?

[jreback]

I think u should convert to i8 if possible (and reverse after) - evn though I guess it's handled as object now

[jreback]

#7560 does this conversion now (actually these are converted to float then coerced back).

So rebase after I merge that and let's take another look

[jreback]

can you show groupby benches after this change..thanks

[sinhrks]

Thanks, but #7560 doesn't convert group keys, correct? Thus, _algos.groupby_xxx functions are still receives objects as labels argument even after #7560.

I think dtype should be converted in Index.groupby before calling cython funcs. Is it ok to change Index.groupby only to accept ndarray, or is it categorized as public API?

[jreback]

Ahh, yes was thinking of something else.

Index.groupby is ONLY used by groupby IIRC, so its not really public (so ok to change).

[sinhrks]

OK, let me try. Maybe there is no side-effect, because currently _algos.groupby_xxx only accepts ndarray (thus the point which error occurrs being changed if something passed other than ndarray).

[sinhrks]

@jreback I made it work when group-key has datelike dtypes, is this what you mean?

Have to consider object dtype case and better way to coercing dtype back.

[jreback]

can you rebase and revisit this...thxs

[jreback]

pushing if you get 2 this in next day or 2 lmk

[jreback]

@sinhrks let's do this for 0.17.0

[sinhrks]

Yes, fixed to work on the latest master.

[jreback]

@sinhrks don't add ANOTHER nat checker, use https://github.com/pydata/pandas/blob/master/pandas/src/inference.pyx#L228

[sinhrks]

Thanks, I haven't noticed that.

[jreback]

this doesn't look right. these are indices, not actual objects, right?

[sinhrks]

Correct, actually no need to change algos.pyx. Fixed.

[sinhrks]

@jreback pls check if there is anything I should do?

[jreback]

lgtm....go ahead and merged

[sinhrks]

Thanks, and sorry I found the doc header alignment is incorrect. Will fix it then merge if nothing else.