BUG: Series rolling count ignores min_periods #30923

fujiaxiang · 2020-01-11T16:27:36Z

closes BUG: Series.rolling min_period is ignored and NA behaves strangely #26996
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

WillAyd

Thanks for the PR. Looks generally good just a question

pandas/core/window/rolling.py

…unt_ignores_min_periods

fujiaxiang · 2020-01-17T16:03:17Z

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.

…unt_ignores_min_periods

pandas/core/window/rolling.py

jreback

ok with this for 1.0.0 if you can move this note

…unt_ignores_min_periods

Updated the behavior of rolling and expanding count so that it becomes consistent with all other rolling and expanding functions. Also updated many test cases to reflect this change of behavior.

fujiaxiang · 2020-01-21T15:17:01Z

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.

WillAyd

I think looks good but given scope of change I don't think this is suitable for 1.0 - it isn't backwards compatible in its current form with the change to how count works, so probably best for 1.1 @jreback ?

pandas/core/window/rolling.py

fujiaxiang · 2020-01-22T15:22:36Z

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 (bfe10f0), @WillAyd thinks the scope of change is too large for V1.0 since last commit (c0046b6).

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?

fujiaxiang · 2020-01-22T15:53:36Z

Is there a way to retrigger checks without new commits? Recently CI seems to randomly fail a lot.

TomAugspurger · 2020-01-22T21:15:26Z

If you merge master & repush then the CI should be fixed.

…unt_ignores_min_periods

mroeschke · 2020-01-24T06:47:29Z

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.

…996)

fujiaxiang · 2020-01-24T07:39:03Z

@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?

fujiaxiang · 2020-01-24T08:08:16Z

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.

mroeschke · 2020-01-24T18:28:06Z

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.

doc/source/whatsnew/v1.0.0.rst

mroeschke · 2020-01-24T18:33:13Z

One small whatsnew comment otherwise LGTM

fujiaxiang · 2020-01-25T02:42:37Z

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.

jreback · 2020-01-25T16:03:49Z

does this fully close #26996 ?

jreback

lgtm. @mroeschke this is a straight bug fix now?

pandas/core/window/rolling.py

jreback · 2020-01-26T00:29:45Z

thanks for the patch @fujiaxiang

jreback · 2020-01-26T01:16:43Z

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).

fujiaxiang · 2020-01-26T01:43:49Z

PR created (#31319). It has some conflict with target branch in .travis.yml, @jreback could you help resolve?

…ndexing-1row-df * upstream/master: (194 commits) DOC Remove Python 2 specific comments from documentation (pandas-dev#31198) Follow up PR: pandas-dev#28097 Simplify branch statement (pandas-dev#29243) BUG: DatetimeIndex.snap incorrectly setting freq (pandas-dev#31188) Move DataFrame.info() to live with similar functions (pandas-dev#31317) ENH: accept a dictionary in plot colors (pandas-dev#31071) PERF: add shortcut to Timestamp constructor (pandas-dev#30676) CLN/MAINT: Clean and annotate stata reader and writers (pandas-dev#31072) REF: define _get_slice_axis in correct classes (pandas-dev#31304) BUG: DataFrame.floordiv(ser, axis=0) not matching column-wise bheavior (pandas-dev#31271) PERF: optimize is_scalar, is_iterator (pandas-dev#31294) BUG: Series rolling count ignores min_periods (pandas-dev#30923) xfail sparse warning; closes pandas-dev#31310 (pandas-dev#31311) REF: DatetimeIndex.get_value wrap DTI.get_loc (pandas-dev#31314) CLN: internals.managers (pandas-dev#31316) PERF: avoid copies if possible in fill_binop (pandas-dev#31300) Add test for multiindex json (pandas-dev#31307) BUG: passing TDA and wrong freq to TimedeltaIndex (pandas-dev#31268) BUG: inconsistency between PeriodIndex.get_value vs get_loc (pandas-dev#31172) CLN: remove _set_subtyp (pandas-dev#31301) CI: Updated version of macos image (pandas-dev#31292) ...

BUG: Series rolling count ignores min_periods (GH26996)

398bf7d

WillAyd requested changes Jan 11, 2020

View reviewed changes

pandas/core/window/rolling.py Outdated Show resolved Hide resolved

WillAyd added the Window rolling, ewma, expanding label Jan 11, 2020

fujiaxiang requested a review from WillAyd January 12, 2020 07:12

fujiaxiang added 2 commits January 17, 2020 20:17

Merge remote-tracking branch 'upstream/master' into series_rolling_co…

5d471c9

…unt_ignores_min_periods

ENH: handles min_periods argument in rolling.count (GH26996)

47f4e61

fujiaxiang added 4 commits January 18, 2020 23:52

DOC: moved whatsnew to V1.1.0

df2a3e9

Merge remote-tracking branch 'upstream/master' into series_rolling_co…

554abfa

…unt_ignores_min_periods

Merge remote-tracking branch 'upstream/master' into series_rolling_co…

c4878b0

…unt_ignores_min_periods

Merge remote-tracking branch 'upstream/master' into series_rolling_co…

b0f5baa

…unt_ignores_min_periods

WillAyd requested changes Jan 20, 2020

View reviewed changes

pandas/core/window/rolling.py Outdated Show resolved Hide resolved

jreback added this to the 1.0.0 milestone Jan 20, 2020

jreback added the Bug label Jan 20, 2020

jreback requested changes Jan 20, 2020

View reviewed changes

fujiaxiang added 4 commits January 21, 2020 10:46

TST: added more tests (GH26996)

bfe10f0

Merge branch 'master' into series_rolling_count_ignores_min_periods

1a90629

Merge remote-tracking branch 'upstream/master' into series_rolling_co…

be0926b

…unt_ignores_min_periods

BUG: updated rolling and expanding count for consistency (GH26996)

c0046b6

Updated the behavior of rolling and expanding count so that it becomes consistent with all other rolling and expanding functions. Also updated many test cases to reflect this change of behavior.

fujiaxiang changed the title ~~BUG: Series rolling count ignores min_periods~~ ENH: Series rolling count ignores min_periods Jan 21, 2020

fujiaxiang requested review from WillAyd and jreback January 21, 2020 15:17

WillAyd reviewed Jan 21, 2020

View reviewed changes

pandas/core/window/rolling.py Outdated Show resolved Hide resolved

fujiaxiang requested a review from WillAyd January 22, 2020 03:48

CLN: further cleaned code (GH26996)

8807ba1

Merge remote-tracking branch 'upstream/master' into series_rolling_co…

7aa1582

…unt_ignores_min_periods

fujiaxiang added 2 commits January 24, 2020 15:28

BUG: changed min_periods default to 0 for rolling and expanding (GH26…

1a4352b

…996)

BUG: reverted non-relevant changes accidentally added (GH26996)

9fa4531

fujiaxiang changed the title ~~ENH: Series rolling count ignores min_periods~~ BUG: Series rolling count ignores min_periods Jan 24, 2020

mroeschke reviewed Jan 24, 2020

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

BUG: small change in whatsnew (GH26996)

987c033

fujiaxiang mentioned this pull request Jan 25, 2020

ENH: Change default behavior of rolling.count to be consistent with others #31302

Closed

fujiaxiang requested a review from mroeschke January 25, 2020 02:42

jreback requested changes Jan 25, 2020

View reviewed changes

pandas/core/window/rolling.py Outdated Show resolved Hide resolved

CLN: slight cleanup (GH26996)

a00532a

jreback approved these changes Jan 26, 2020

View reviewed changes

jreback merged commit 823bf6c into pandas-dev:master Jan 26, 2020

fujiaxiang mentioned this pull request Jan 26, 2020

BUG Series rolling count ignores min periods #31319

Closed

4 tasks

fujiaxiang mentioned this pull request Jan 26, 2020

Backport PR: Series rolling count ignores min_periods #31320

Merged

fujiaxiang deleted the series_rolling_count_ignores_min_periods branch January 26, 2020 04:45

ueshin mentioned this pull request Feb 12, 2020

Support pandas >=1.0.0 databricks/koalas#1197

Merged

brandon-b-miller mentioned this pull request Mar 18, 2020

[REVIEW] Support Pandas 1.0+ rapidsai/cudf#4546

Merged

13 tasks

brandon-b-miller mentioned this pull request May 29, 2020

BUG: Rolling count aggregation produces unexpected results #34466

Open

3 tasks

brandon-b-miller mentioned this pull request Jun 25, 2020

[BUG] Rolling count aggregations produce different results than Pandas 1.0+ rapidsai/cudf#5580

Open

mythrocks mentioned this pull request Sep 30, 2020

[REVIEW] Fix rolling-window count for null input rapidsai/cudf#6344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Series rolling count ignores min_periods #30923

BUG: Series rolling count ignores min_periods #30923

fujiaxiang commented Jan 11, 2020 •

edited

Loading

WillAyd left a comment

fujiaxiang commented Jan 17, 2020

jreback left a comment

fujiaxiang commented Jan 21, 2020

WillAyd left a comment

fujiaxiang commented Jan 22, 2020

fujiaxiang commented Jan 22, 2020

TomAugspurger commented Jan 22, 2020

mroeschke commented Jan 24, 2020

fujiaxiang commented Jan 24, 2020 •

edited

Loading

fujiaxiang commented Jan 24, 2020

mroeschke commented Jan 24, 2020

mroeschke commented Jan 24, 2020

fujiaxiang commented Jan 25, 2020

jreback commented Jan 25, 2020

jreback left a comment

jreback commented Jan 26, 2020

jreback commented Jan 26, 2020

fujiaxiang commented Jan 26, 2020

BUG: Series rolling count ignores min_periods #30923

BUG: Series rolling count ignores min_periods #30923

Conversation

fujiaxiang commented Jan 11, 2020 • edited Loading

WillAyd left a comment

Choose a reason for hiding this comment

fujiaxiang commented Jan 17, 2020

jreback left a comment

Choose a reason for hiding this comment

fujiaxiang commented Jan 21, 2020

WillAyd left a comment

Choose a reason for hiding this comment

fujiaxiang commented Jan 22, 2020

fujiaxiang commented Jan 22, 2020

TomAugspurger commented Jan 22, 2020

mroeschke commented Jan 24, 2020

fujiaxiang commented Jan 24, 2020 • edited Loading

fujiaxiang commented Jan 24, 2020

mroeschke commented Jan 24, 2020

mroeschke commented Jan 24, 2020

fujiaxiang commented Jan 25, 2020

jreback commented Jan 25, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback commented Jan 26, 2020

jreback commented Jan 26, 2020

fujiaxiang commented Jan 26, 2020

fujiaxiang commented Jan 11, 2020 •

edited

Loading

fujiaxiang commented Jan 24, 2020 •

edited

Loading