BUG: Series rolling count ignores min_periods
State: closed

- [x] closes #26996
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: Window
description: rolling, ewma, expanding

Comments:
As mentioned in my previous reply to a change request, I think we should properly handles `min_periods` when it is explicitly set by user, but the default behavior should remain the same, i.e. `min_periods=0` by default (it defaults to `None` in method signature but inside is set to 0). 

The reason is that this `count` method is meant to count the non-NAs values in the window, and we don't want to require all values in the window to be valid in order to produce a valid count, which is what other similar APIs do (e.g. rolling.mean sets `min_periods=window`, effectively requires all values in the window to be valid, otherwise produces `NaN`)

However, if you feel strongly we should make the default behavior exactly the same as other APIs like `rolling.mean`, let me know and I will update this PR. I have the code ready. The new code updates 63 existing test cases because of the change in default behavior.
After further investigation, I realize we can in fact make `count` function behave consistently with other `rolling` and `expanding` functions.

I have pushed the code changes, and also updated many test cases that were "incorrect".

Now the `count` function correctly handles ``min_periods`` argument, which was ignored previously.
After a few rounds of debugging and code clean-up, now `rolling.count` behaves completely consistent with other APIs (e.g `rolling.mean`).

@jreback mentioned the fix is ok for V1.0 at 2 commits ago (bfe10f08696d0073db27bac898d6629fbef09f5d), @WillAyd thinks the scope of change is too large for V1.0 since last commit (c0046b6f55c68ffc9d63bf8624fc1e926a4b00df).

Would it be a good idea to roll-out the backward-compatible smaller fix at 2 commits ago for V1.0, and the longer-term complete fix for V1.1 with a separate PR?
Is there a way to retrigger checks without new commits? Recently CI seems to randomly fail a lot.
If you merge master & repush then the CI should be fixed.
@TomAugspurger 
That's what I have been trying to do, but it doesn't always work. Sometimes I have to do that a few times before the checks go green. The problem is, I will have to wait for the master to have new commits and doing that a few times may cost me one or two days. Is there anyway I can reliably re-trigger checks without having to push new commits (including merge master)?
to retrigger a build if u think it’s actually an intermittent issue (normally pretty rare)
you can close and open the PR
@mroeschke if you'd have a look here
> I suspect that originally that `min_periods` was intentionally set `0` to make `count` consistent with the non-rolling `count` methods (i.e. always count all the non-NA values), but overall I think this is an okay change since `rolling` has the ability to handle a `min_periods` concept and the default as you mentioned is set to `0`.
> 
> Just some small testing comments.

@mroeschke Note that we have changed the default value of ``min_periods`` in the latest commits. Now it defaults to same value as ``window``, same as other APIs. It was still 0 a few commits ago (bfe10f08696d0073db27bac898d6629fbef09f5d).
Ah thanks for highlighting that @fujiaxiang. Sorry for not totally following the entire conversation to date.

I have a slight preference on keeping the default `min_periods` for `count` to be `0` instead of `window` so `count` is consistent with `count` functions on other pandas objects (e.g. `DataFrame.count`.

Otherwise, I do agree with @WillAyd and think changing the default `min_periods` is a bit aggressive to include 1.0.
 
@mroeschke Would it be a good idea to roll out commit (bfe10f08696d0073db27bac898d6629fbef09f5d) for V1.0, and then separately roll out latest commit for V1.1?
I think we would still need some sort of `DeprecationWarning` on `count` or `min_periods` (although the parameter wasn't working before) since the default behavior of the function would change with the latest commit you have.

Another reason why it may just be simpler to keep the default `min_periods` to 0 and have the PR be a bug fix that `min_periods` now is respected.
@mroeschke I have changed default ``min_periods`` back to 0. Could you review?

I have kept most of the changes in the test cases, which now explicitly specifies `min_periods` (`min_periods=0` mostly, `min_periods=min_periods` otherwise where the effect is the same and consistent with others). This would make it easier when we decide to go-ahead with changing the default behavior in future versions.

Should I also create a new issue to add the `DeprecationWarning` and the actual change for next version?



> Ah thanks for highlighting that @fujiaxiang. Sorry for not totally following the entire conversation to date.
> 
> I have a slight preference on keeping the default `min_periods` for `count` to be `0` instead of `window` so `count` is consistent with `count` functions on other pandas objects (e.g. `DataFrame.count`.
> 
> Otherwise, I do agree with @WillAyd and think changing the default `min_periods` is a bit aggressive to include 1.0.

@mroeschke btw changing the default ``min_periods`` doesn't make the result that different from `DataFrame.count`. Unlike `rolling.mean` or `rolling.sum` etc, the `min_periods` argument for `rolling.count` only takes effect on the window size, and doesn't care how many NA values are in the window. If we change its default value to the same as `window`, same as other APIs, we would only see NAs in the first few rows (which would be obvious to user that this is caused by `min_periods`), and the rest are consistent.
Yes please create a new issue for possibly changing the default behavior of `rolling.count` with respect to `min_periods`. Good to discuss the change outside this PR.
One small whatsnew comment otherwise LGTM
> Yes please create a new issue for possibly changing the default behavior of `rolling.count` with respect to `min_periods`. Good to discuss the change outside this PR.

Created new issue #31302. Let me know what you guys think there.
does this fully close #26996 ?
thanks for the patch @fujiaxiang 
hmm, this was supposed to backport. very odd. @fujiaxiang can you push a PR to the 1.0.x branch as well (the bot is supposed to do this).
PR created (#31319). It has some conflict with target branch in .travis.yml, @jreback could you help resolve?