PERF: improves performance in SeriesGroupBy.count

[behzadnouri]

BUG: closes bug in Series.count when index has nulls

In [4]: ts
Out[4]:
a  1      0
   2      1
b  2      2
   NaN    3
c  1      4
   2      5
dtype: int64

In [5]: ts.count(level=1)
Out[5]:
1    2
2    4          # <<< BUG!
dtype: int64

In [6]: from string import ascii_lowercase

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

In [8]: n = 1 << 21

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

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

In [11]:

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

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

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

on branch:

In [5]: ts.count(level=1)
Out[5]:
 1     2
 2     3
NaN    1
dtype: int64

...

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

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

[jreback]

rather than adding this almost identical code, doesnt it make sense to:

return self.groupby(level=level).count().__finalize__(self) ?

for the level not None case

[behzadnouri]

this is just removing the bug from current implementation. i don't think having an optional level argument would be useful here as it is basically equivalent to groupby on level. that said, groupby removes nulls from keys:

In [5]: ts
Out[5]: 
a  1      0
   2      1
b  2      2
   NaN    3
c  1      4
   2      5
dtype: int64

In [6]: ts.groupby(level=1).count()
Out[6]: 
1    2
2    3
dtype: int64

[jreback]

not sure I understand. the expression I gave above is equivalent to what you wrote, yes? its is just dispatching to the groupby impl. (which is how all of the other stat functions which accept level work).

[behzadnouri]

the expression I gave above is equivalent to what you wrote, yes?

yes, if index does not include nan.

[jreback]

my point is why we having a special case implementation, when simply using s.groupby(level=level).count() is acceptable?

and this is what all of the other make_stat_* functions do.

Since this was a bug nothing is even being eliminated.

[jreback]

@behzadnouri pls rebase and make the changes as above

[jreback]

@behzadnouri can you update according to comments

[behzadnouri]

@jreback the added test will fail if i change it what u suggest.

[jreback]

@behzadnouri I just don't think its worth it to support this kind of behavior in Series.count which is inconsitent with all other stat functions on how they handle levels, they just dispatch to groupby.

[behzadnouri]

then plz go ahead and change it

[jreback]

#443 would fix this, e.g. we need an option like:

s.count(level=1, skipna=False)

[jreback]

hmm, looks like my way would break some existing tests....ok will merge as is, and when we eventually add nan group handling in groupby can simplify this code.

[behzadnouri]

need rebase?

[jreback]

I just did it. thxs.

[jreback]

merged via 33723f9