COMPAT: sum/prod on all nan will remain nan regardless of bottleneck install
State: closed

xref #15507
closes #9422


Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: Compat
description: pandas objects compatability with Numpy or Python functions

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

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

```diff
@@            Coverage Diff             @@
##           master   #17630      +/-   ##
==========================================
- Coverage   91.19%   91.17%   -0.03%     
==========================================
  Files         163      163              
  Lines       49652    49651       -1     
==========================================
- Hits        45282    45269      -13     
- Misses       4370     4382      +12
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.96% <100%> (-0.01%)` | :arrow_down: |
| #single | `40.18% <23.07%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `100% <ø> (ø)` | :arrow_up: |
| [pandas/core/nanops.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbmFub3BzLnB5) | `96.67% <100%> (-0.99%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.77% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.04% <0%> (+0.05%)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `95.02% <0%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17630?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/17630?src=pr&el=footer). Last update [d1fe892...6f56342](https://codecov.io/gh/pandas-dev/pandas/pull/17630?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/17630?src=pr&el=h1) Report
> Merging [#17630](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/3ba2cffff4c851460d3a2dbe893955dc9d9d0640?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #17630      +/-   ##
==========================================
- Coverage   91.24%   91.21%   -0.03%     
==========================================
  Files         163      163              
  Lines       49994    49993       -1     
==========================================
- Hits        45615    45602      -13     
- Misses       4379     4391      +12
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.02% <100%> (-0.01%)` | :arrow_down: |
| #single | `40.24% <23.07%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.09% <ø> (+0.05%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `100% <ø> (ø)` | :arrow_up: |
| [pandas/core/nanops.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbmFub3BzLnB5) | `96.67% <100%> (-0.99%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.74% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.98% <0%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17630?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/17630?src=pr&el=footer). Last update [3ba2cff...3db01c9](https://codecov.io/gh/pandas-dev/pandas/pull/17630?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

cc @shoyer 

after all of the conversation of #9422 dead silence here.
any objections to context given discussion in #9422 

@jorisvandenbossche @shoyer 
> What is the performance impact of not using bottleneck? From a quick test it seems quite a bit faster than our own implementation.
But I suppose checking for the case of all NaNs (to then not use bottleneck) will defeat this performance gain?

there is some benefit, can be up to 2x faster (in ad-hoc tests). But again, we *do* more so its not entirely fair (e.g. handle proper dtypes, infs and things like that). But for some straightforward stuff its fine. 
> there is some benefit, can be up to 2x faster (in ad-hoc tests). But again, we do more so its not entirely fair (e.g. handle proper dtypes, infs and things like that)

That is true, but depending on the dtype this extra 'work' is not needed. Eg we could special case integers? As there can never be NaN handling needed. Small benchmark:

```
In [24]: df = pd.DataFrame(np.random.randint(0, 1000, (10000, 10)))

In [27]: pd.options.compute.use_bottleneck = True

In [28]: %timeit df.sum()
241 µs ± 1.05 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [29]: pd.options.compute.use_bottleneck = False

In [30]: %timeit df.sum()
845 µs ± 4.35 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```

So it is a considerable difference. 
thats a whopping 3us. If you want to do a PR, go ahead.
I can easily make that number bigger by making a bigger frame :-) it's the relative number that is relevant. 
But to the point, I am not familiar with the nanops.py code, but it's not adding a `is_integer_dtype` check here? https://github.com/jreback/pandas/blob/3c91148b1f5b48fa97e475c63a5adfeb429fbdf6/pandas/core/nanops.py#L166 (there is already a check for object and datetime dtype)
Not sure it's worth the complexity in implementation, but _could_ optimistically use bottleneck, and only if the identity is returned (0 or 1) do a check for all `nan`
note by always using our own routine we also avoid some buggy versions on windows  (#15507) though might be fixed in later versions
update with more documentation. @jorisvandenbossche @shoyer @TomAugspurger 
@jorisvandenbossche @TomAugspurger any more comments
updated
@jreback I had one comment in my last review that I think got lost with bad markdown formatting:

> Can we confirm the behavior for summing an empty series with `skipna=False`? e.g.,  `pd.Series([]).sum(skipna=False)`?

This should be NaN, not 0 like NumPy.
0.20.3 
```
# with bottleneck
In [1]: pd.Series([]).sum(skipna=True)
Out[1]: 0

In [2]: pd.Series([]).sum(skipna=False)
Out[2]: 0

In [3]: pd.Series([np.nan]).sum(skipna=True)
Out[3]: 0.0

In [4]: pd.Series([np.nan]).sum(skipna=False)
Out[4]: nan

# no bottleneck
In [1]: pd.Series([]).sum(skipna=True)
Out[1]: 0

In [2]: pd.Series([]).sum(skipna=False)
Out[2]: 0

In [3]: pd.Series([np.nan]).sum(skipna=True)
Out[3]: nan

In [4]: pd.Series([np.nan]).sum(skipna=False)
Out[4]: nan

```

PR
```
In [1]: pd.Series([]).sum(skipna=True)
Out[1]: nan

In [2]: pd.Series([]).sum(skipna=False)
Out[2]: nan

In [3]: pd.Series([np.nan]).sum(skipna=True)
Out[3]: nan

In [4]: pd.Series([np.nan]).sum(skipna=False)
Out[4]: nan
```
Thanks, that looks right!

On Tue, Oct 10, 2017 at 3:16 AM Jeff Reback <notifications@github.com>
wrote:

> 0.20.3
>
> # with bottleneck
> In [1]: pd.Series([]).sum(skipna=True)
> Out[1]: 0
>
> In [2]: pd.Series([]).sum(skipna=False)
> Out[2]: 0
>
> In [3]: pd.Series([np.nan]).sum(skipna=True)
> Out[3]: 0.0
>
> In [4]: pd.Series([np.nan]).sum(skipna=False)
> Out[4]: nan
>
> # no bottleneck
> In [1]: pd.Series([]).sum(skipna=True)
> Out[1]: 0
>
> In [2]: pd.Series([]).sum(skipna=False)
> Out[2]: 0
>
> In [3]: pd.Series([np.nan]).sum(skipna=True)
> Out[3]: nan
>
> In [4]: pd.Series([np.nan]).sum(skipna=False)
> Out[4]: nan
>
>
> PR
>
> In [1]: pd.Series([]).sum(skipna=True)
> Out[1]: nan
>
> In [2]: pd.Series([]).sum(skipna=False)
> Out[2]: nan
>
> In [3]: pd.Series([np.nan]).sum(skipna=True)
> Out[3]: nan
>
> In [4]: pd.Series([np.nan]).sum(skipna=False)
> Out[4]: nan
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/17630#issuecomment-335348060>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABKS1vzpxhQ4Zz589NUueOhyHGKt7gMMks5squGHgaJpZM4Pg12h>
> .
>

k, merging on green.
@jreback Thanks a lot for pushing for this!