ENH: Added a min_count keyword to stat funcs
State: closed

The current default is 1, reproducing the behavior of pandas 0.21. The current
test suite should pass. I'll add additional commits here changing the default to be 0.

Currently, only nansum and nanprod actually do anything with `min_count`. It
will not be hard to adjust other nan* methods use it if we want. This was just
simplest for now.

Additional tests for the new behavior have been added.

closes #18678

Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: API Design
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations

Comments:
https://github.com/pandas-dev/pandas/pull/18876/commits/5630d9d167994632e6307acae78e8dd33b095b9c passed all the CI tests, so that's good.

I just pushed another commit that fixes a few things, but is still backwards compatible with 0.21.x.

I'll push my breaking changes once I get things passing locally.
I think we should for now only add `min_count` to `sum` and `prod`. Or does that complicate the implementation?
> Or does that complicate the implementation?

Not much. I mainly did it for all of them since it was easier, and would *possibly* be useful for other reductions (haven't thought it through). I'll push a commit that does it just for sum and prod.
For other reductions like min, max, std, .. we already return NaN for empty or all-NA series, so not sure what `min_count` would be for
Pushed another backwards compatible commit, including docstrings, written from the point of view of a hypothetical 0.21.2 release. I haven't through through whether it'd be worth backporting the non-breaking changes (adding `min_count=1`) to 0.21.x, but in case we do it'll just be the current set of commits:


```python
Signature: pd.Series.sum(self, axis=None, skipna=None, level=None, numeric_only=None, min_count=1, **kwargs)
Docstring:
Return the sum of the values for the requested axis

Parameters
----------
axis : {index (0)}
skipna : boolean, default True
    Exclude NA/null values. If an entire row/column is NA or empty, the result
    will be NA
level : int or level name, default None
    If the axis is a MultiIndex (hierarchical), count along a
    particular level, collapsing into a scalar
numeric_only : boolean, default None
    Include only float, int, boolean columns. If None, will attempt to use
    everything, then use only numeric data. Not implemented for Series.
min_count : int, default 1
    The required number of valid values to perform the operation. If fewer than
    ``min_count`` non-NA values are present the result will be NA.

    .. versionadded :: 0.21.2

       Added with the default being 1. This means the sum or product
       of an all-NA or empty series is ``NaN``.

Returns
-------
sum : scalar or Series (if level specified)

Examples
--------
By default, the sum of an empty series is ``NaN``.

>>> pd.Series([]).sum()  # min_count=1 is the default
nan

This can be controlled with the ``min_count`` parameter. For example, if
you'd like the sum of an empty series to be 0, pass ``min_count=0``.

>>> pd.Series([]).sum(min_count=0)
0.0

Thanks to the ``skipna`` parameter, ``min_count`` handles all-NA and
empty series identically.

>>> pd.Series([np.nan]).sum()
nan

>>> pd.Series([np.nan]).sum(min_count=0)
0.0
```
For reviewing, do people have a preference for 

a.) 2 PRs: the first adding a backwards compatible `min_count=1`, the second changing `min_count=0` and updating all the tests
b.) A single PR (maybe with me rebasing / squashing to keep the two changes separate).
> For reviewing, do people have a preference for

> a.) 2 PRs: the first adding a backwards compatible min_count=1, the second changing min_count=0 and updating all the tests
> b.) A single PR (maybe with me rebasing / squashing to keep the two changes separate).

a) maybe that is better, then we can see both passing
I think this is ready for review (cc @shoyer). I haven't written a whatsnew since I'll be doing that in the followup PR.

Just to verify, the current plan is to release 0.22.0 as 0.21.1 + this PR and my follow PR with the breaking changes?
FYI, I have a `min_count` keyword working for groupby on my next branch:

```python
In [1]: import pandas as pd

In [2]: df = pd.DataFrame({"A": pd.Categorical(['a', 'a'], categories=['a', 'b']), "B": [1, 1]})

In [3]: df.groupby("A").B.sum()
Out[3]:
A
a    2
b    0
Name: B, dtype: int64

In [4]: df.groupby("A").B.sum(min_count=1)
Out[4]:
A
a    2.0
b    NaN
Name: B, dtype: float64

```

I think I'll include that in this PR as well (with the default being 1).
Added min_count to `resample` in the last push.

```python
In [4]: s.resample('30T').sum()  # default min_count=1 for now, will change to 0
Out[4]:
2017-01-01 00:00:00    1.0
2017-01-01 00:30:00    NaN
2017-01-01 01:00:00    1.0
Freq: 30T, dtype: float64

In [5]: s.resample('30T').sum(min_count=0)
Out[5]:
2017-01-01 00:00:00    1
2017-01-01 00:30:00    0
2017-01-01 01:00:00    1
Freq: 30T, dtype: int64
```

I'm going to see how difficult `rolling` is now. I think for symmetry it'd be nice to have, but how does it interact with `min_periods`?

I believe that `min_periods` specifies required length of the *window*. If the window is shorter than that, the output is NA regardless of `min_count`. 

Given a widow at least `min_periods` length, `min_count` specifies the required number of non-NA observation for a non-NA output.
> I'm going to see how difficult rolling is now. I think for symmetry it'd be nice to have, but how does it interact with min_periods?

> I believe that min_periods specifies required length of the window. If the window is shorter than that, the output is NA regardless of min_count.

> Given a widow at least min_periods length, min_count specifies the required number of non-NA observation for a non-NA output.

``min_count`` is de-facto the same as ``min_periods`` or at least it performs the same function. I don't think it is necessary to add and would add much complexity and user confusion. We *could* just rename ``min_periods`` to ``min_count`` I suppose. (need a deprecation cycle).

Ah, is `min_periods` the minimum number of observations, or the minimum number of non-NA observations? e.g.

```python
In [2]: s = pd.Series(1, index=pd.date_range("2017", periods=6, freq="D"))
   ...: t = s.copy()
   ...: t.iloc[[3, 4]] = np.nan
   ...:

In [3]: s
Out[3]:
2017-01-01    1
2017-01-02    1
2017-01-03    1
2017-01-04    1
2017-01-05    1
2017-01-06    1
Freq: D, dtype: int64

In [4]: t
Out[4]:
2017-01-01    1.0
2017-01-02    1.0
2017-01-03    1.0
2017-01-04    NaN
2017-01-05    NaN
2017-01-06    1.0
Freq: D, dtype: float64

In [5]: s.rolling(2).sum()
Out[5]:
2017-01-01    NaN
2017-01-02    2.0
2017-01-03    2.0
2017-01-04    2.0
2017-01-05    2.0
2017-01-06    2.0
Freq: D, dtype: float64

In [6]: t.rolling(2).sum()
Out[6]:
2017-01-01    NaN   # (1,)
2017-01-02    2.0   # (1, 1)
2017-01-03    2.0   # (1, 1)
2017-01-04    NaN   # (1, nan)
2017-01-05    NaN   # (nan, nan)
2017-01-06    NaN   # (nan, 1)
Freq: D, dtype: float64
```

So will `Out[6]` be changing at all?
> Groupby seems to have a wrong default (for now to be backwards compatible), but can't directly spot the error in the code:

I just pushed a commit fixing that. `min_count` wasn't being passed through.

```python
In [4]: s.groupby(level=0).sum()
Out[4]:
1    1.0
2    NaN
dtype: float64
```

> As also commented inline somewhere, the sum(level=) does not seem to be implemented yet:

This seems to be fixed (presumably `level=` is using groupy and was hit by the same issue)

```python
In [10]: s.sum(level=0)
Out[10]:
1    1.0
2    NaN
dtype: float64

```
Groupby is indeed fixed!

> This seems to be fixed (presumably level= is using groupy and was hit by the same issue)

The default is indeed fixed, but `min_count` has not yet any effect, so that still needs to be passed through as well in some way I think
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18876?src=pr&el=h1) Report
> Merging [#18876](https://codecov.io/gh/pandas-dev/pandas/pull/18876?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/ef753900b5c3956c43068ff64d271e1332024f52?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #18876      +/-   ##
==========================================
+ Coverage   91.57%   91.65%   +0.07%     
==========================================
  Files         150      154       +4     
  Lines       48913    51394    +2481     
==========================================
+ Hits        44794    47107    +2313     
- Misses       4119     4287     +168
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.52% <100%> (-0.42%)` | :arrow_down: |
| #single | `40.84% <49.12%> (-0.33%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18876?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.95% <100%> (+0.03%)` | :arrow_up: |
| [pandas/core/nanops.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbmFub3BzLnB5) | `96.68% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/resample.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzYW1wbGUucHk=) | `96.38% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.13% <100%> (+0.04%)` | :arrow_up: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `92.08% <0%> (-0.48%)` | :arrow_down: |
| [pandas/tseries/offsets.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvb2Zmc2V0cy5weQ==) | `96.95% <0%> (-0.12%)` | :arrow_down: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `92.93% <0%> (-0.07%)` | :arrow_down: |
| [pandas/core/indexes/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZWxpa2UucHk=) | `97.13% <0%> (-0.07%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.44% <0%> (-0.01%)` | :arrow_down: |
| ... and [16 more](https://codecov.io/gh/pandas-dev/pandas/pull/18876/diff?src=pr&el=tree-more) | |

------

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

Ok, we should be good on series/frame, groupby, and resample. Does anyone have thoughts on expanding / window? Does `min_periods` mean

1. The minimum number of observations in the window
2. The minimum number of *non-NA* observations in the window

If it's 1, then the current behavior (on master) is buggy:

```python
In [6]: s = pd.Series(1, index=pd.date_range("2017", periods=6, freq="D"))
   ...: t = s.copy()
   ...: t.iloc[[3, 4]] = np.nan
   ...:

In [7]: t.rolling(2, min_periods=2).apply(lambda x: np.nansum(x))
Out[7]:
2017-01-01    NaN  # (1,)
2017-01-02    2.0  # (1, 1)
2017-01-03    2.0  # (1, 1)
2017-01-04    NaN  # (1, nan)
2017-01-05    NaN  # (nan, nan)
2017-01-06    NaN  # (nan, 1)
Freq: D, dtype: float64
```
why would you think 1)? it always has meant non-NA 
I agree it currently means non-NA rows (although that is not explicit in the docstring). 

I am thinking if there are possible reasons we would want to change this, and to have both `min_periods` in `rolling` and `min_count` in `sum`. In principle that would give the ability to distinguish between actuel NaNs in the data and no data in the period ("introduced" NaNs). 
(and for all other functions apart from sum and prod, it wouldn't change anything as NaN values always return NaN anyway, so whether it is because of a NaN in the data or because of `min_periods` no including those NaN data, that does not matter).

I personally wouldn't rename `min_periods` to `min_count`, but just leave the name as is (also if we don't add a `min_count` to the rolling `sum`). It is in a different function (`rolling`, not `sum`) and is also relevant for other aggregations that don't have such a `min_count` in their normal dataframe counterpart.
Agreed with @jorisvandenbossche.

There may be a bit of a slight issue with the current implementation of `rolling.sum()` when `min_periods=0`.

On my other branch:

```python
In [9]: x = pd.Series([np.nan])

In [10]: x.rolling(1, min_periods=0).sum()

Out[10]:
0   NaN
dtype: float64

In [11]: x.rolling(1, min_periods=0).apply(lambda v: pd.Series(v).sum())

Out[11]:
0    0.0
dtype: float64
```

I assume we want those to be consistent?
Here's master:

```python
In [1]: import pandas as pd; import numpy as np;

In [2]: x = pd.Series([np.nan])

In [3]: x.rolling(1, min_periods=0).sum()
Out[3]:
0   NaN
dtype: float64

In [4]: x.rolling(1, min_periods=1).sum()
Out[4]:
0   NaN
dtype: float64

```

Here's my branch

```python
In [1]: import pandas as pd; import numpy as np;

In [2]: x = pd.Series([np.nan])

In [3]: x.rolling(1, min_periods=0).sum()
Out[3]:
0    0.0
dtype: float64

In [4]: x.rolling(1, min_periods=1).sum()
Out[4]:
0   NaN
dtype: float64

In [5]: x.rolling(1).sum()
Out[5]:
0   NaN
dtype: float64
```

The fix in `window.pyx` is small. I don't think it's worth including here, since the behavior on master is correct for the 0.21 semantics of "sum of all-missing is missing".
> The fix in window.pyx is small. I don't think it's worth including here, since the behavior on master is correct for the 0.21 semantics of "sum of all-missing is missing".

To be clear: so for now no change, which is keep min_periods as it was, and for now no min_count added to rolling sum?
(which is fine for me)
If we interpret min_periods as the minimum number of non-na periods, then yes. There won’t be a need for rolling.sum to take min_count.

________________________________
From: Joris Van den Bossche <notifications@github.com>
Sent: Saturday, December 23, 2017 8:05:55 AM
To: pandas-dev/pandas
Cc: Tom Augspurger; Author
Subject: Re: [pandas-dev/pandas] ENH: Added a min_count keyword to stat funcs (#18876)


The fix in window.pyx is small. I don't think it's worth including here, since the behavior on master is correct for the 0.21 semantics of "sum of all-missing is missing".

To be clear: so for now no change, which is keep min_periods as it was, and for now no min_count added to rolling sum?
(which is fine for me)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/18876#issuecomment-353727803>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIr1ucILhc6rd72DpuGQSd7Yn-ObQks5tDQjDgaJpZM4RIuoM>.

@TomAugspurger why don't you rebase on master. can merge this on green (as I think you are taking care of any remaining comments in other PR). (and then backport, maybe create the new branch as well)?
Yeah, sounds good, this one should be OK.

I'll create a 0.22.x branch that starts at 0.21.x and backport this + the other PR once that's finished.
@TomAugspurger I also think that need to fix the version number? (or I suppose once you tag 0.22, then can fix that)?
Yeah, how should we do that? I suppose we could tag 0.23.dev0 on master any time right? So then it stays ahead of 0.22.x.
yeah I think can change the tag on master. merge away when ready.
thanks @TomAugspurger