BUG: Fix issue with incorrect groupby handling of NaT

[larvian]

closes #10590

For groupby the time stamps gets converted to integervalue tslib.iNaT
which is -9223372036854775808. The aggregation is then done using this
value with incorrect result as a consequence. The solution proposed here
is to replace its value by np.nan in case it is a datetime64[ns]

[jreback]

test would be nice

[larvian]

I have tried to add a test and update to doc. I am not very sure I have done this correctly :)

[larvian]

I don't know what the next step here is? Am I supposed to do something now or am I waiting for someone to have a look at what I have done? There is a This branch has conflicts that must be resolved Can I see what the conflicts are and fix them?

[max-sixty]

@larvian rebase your branch on upstream master to resolve the conflicts

[larvian]

@MaximilianR Thanks! I will try to figure out how to do that :)

[jreback]

lots of tips here

[sinhrks]

Also, can you add tests for timedelta which looks affected also?

df = pd.DataFrame({'IX': ['A', 'A'], 'num': [np.nan, 100],
                  'dt': [np.nan, datetime.datetime.now()],
                  'td':[np.nan, datetime.timedelta(days=1)]})
# OK
df.min()
# IX                              A
# dt    2015-07-25 07:41:47.871654
# num                           100
# td                1 days 00:00:00
# dtype: object

# NG
# df.groupby('IX').min()
#     dt  num  td
# IX             
# A  NaT  100 NaT

I think you can use com.is_datetime_or_timedelta_dtype to check dtype.

• https://github.com/pydata/pandas/blob/master/pandas/core/common.py#L2517

[larvian]

@jreback Thanks!
@sinhrks Ok I will look at that too.

[larvian]

Hm... I can't get this rebase to work. It gives conflicts which seems to come back after I fix :(
It seems to get messier the more I try.
Perhaps it is easiest to create a new pandas clone and apply my changes there?
I have to leave it for another day.

[TomAugspurger]

We can help walk you through the git problems. Could be easier in the chat room: https://gitter.im/pydata/pandas

[larvian]

Thanks Tom!
I think I need it :)

[larvian]

Just curious, what's next with this PR?
For now is there anything else I should do?

[jreback]

I'd like to do this differently.

move the ._try_operate call to inside .aggregate (its line 1533). Then you can use do the iNaT to np.nan there

[larvian]

Ok,
Then I assume I would do the ._try_operate after checking is_datetime_or_timedelta_dtype not to lose dtype.
But ._try_operate on DatetimeBlock is just values.view('i8')
However the code just after is_datetime_or_timedelta_dtype is values = values.view('int64')
I assume both are not needed so I can remove ._try_operate
Hope this doesn't destroy something else.

[jreback]

wrap this line
use df as a variable name (or maybe df_test)

[jreback]

add the issue number as a comment

[jreback]

can you rebase / update

[larvian]

Sure... rebase done. Queued for Travis CI

[larvian]

Rebased yesterday again and it passed Travis CI.
Today again I have "This branch has conflicts that must be resolved".
This seem to happen all the time due to changes to v0.17.0.txt .
It seems wasteful of Travis time to run CI just because updates due to changes to v0.17.0.txt.
How to avoid the constant conflicts due to changes in this file?
This time I try to insert my line above the last line.

[jreback]

to avoid conflicts, don't insert the line at the end of the notes, put it somewhere else above. This is why I have left lots of blank space (which gradually gets filled up).

[jreback]

ping when green.

[larvian]

This one is green now

[jreback]

thanks!

[behzadnouri]

this added line has broken cython aggregation. You cannot assign nan to an int64 view of an array.

before this patch:

In [1]: from string import ascii_lowercase

In [2]: np.random.seed(2718281)

In [3]: n = 1 << 21

In [4]: dr = date_range('2015-08-30', periods=n // 10, freq='T')

In [5]: df = DataFrame({
   ...:         '1st':np.random.choice(list(ascii_lowercase), n),
   ...:         '2nd':np.random.randint(0, 5, n),
   ...:         '3rd':np.random.choice(dr, n)})

In [6]: df.loc[np.random.choice(n, n // 10), '3rd'] = np.nan

In [7]: gr = df.groupby(['1st', '2nd'])

In [8]: %timeit gr.count()
The slowest run took 21.22 times longer than the fastest. This could mean that an intermediate result is being cached 
1 loops, best of 3: 13.3 ms per loop

In [9]: %timeit gr.count()
100 loops, best of 3: 13.8 ms per loop

In [10]: pd.__version__
Out[10]: '0.16.2+521.g207efc2'

with this patch:

In [8]: %timeit gr.count()
1 loops, best of 3: 144 ms per loop

In [9]: %timeit gr.count()
10 loops, best of 3: 149 ms per loop

In [10]: pd.__version__
Out[10]: '0.16.2+522.g9c2d1a6'

[jreback]

yes that should be iNaT

odd that the tests didn't pick this up

[behzadnouri]

@jreback it is modifying values which are already iNaT:

values[values == tslib.iNaT] = np.nan

[jreback]

this should already be a float here

[jreback]

ahh I see it is raising, being cause and taking the slow path...ok.thanks for the catch

[larvian]

@behzadnouri @jreback Thanks for pointing out the problem.
Is it that the cython aggregation function accepts int64 does not like the nan and raises exception?
Not sure how to deal with it. Should all the NaT be dropped instead before aggregating?
Are there some aggregations where the NaT are important?