Skip to content

BUG: Mitigate division with zero in roll_var #42459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Jul 28, 2021
Merged

BUG: Mitigate division with zero in roll_var #42459

merged 9 commits into from
Jul 28, 2021

Conversation

benHeid
Copy link
Contributor

@benHeid benHeid commented Jul 9, 2021

@benHeid benHeid mentioned this pull request Jul 9, 2021
Closed
3 tasks
jreback
jreback requested changes Jul 9, 2021
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add tests that replicate this (e.g. use your example), they should fail w/o the change and pass with it

jreback
jreback reviewed Jul 9, 2021
View reviewed changes
pandas/_libs/window/aggregations.pyx
if nobs[0]:
mean_x[0] = mean_x[0] + delta / nobs[0]
else:
mean_x[0] = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this zero? and not NaN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that if nobs is zero a new group starts, so I reset mean_x to zero. Analog to the initialization of mean_x in roll_var (compare with the initalization in line 349). However, I am not sure if this is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off the top of my head, this would also need testing when min_periods is 0 or len(array) as well to check if this is correct

@jreback jreback added the Window rolling, ewma, expanding label Jul 9, 2021
@jreback
Copy link
Contributor

jreback commented Jul 9, 2021

cc @mroeschke

@rhshadrach
Copy link
Member

@benHeid

However, I am unsure about writing tests for pandas since this is my first PR to pandas. I would appreciate help here. Thank you.

Take a look at

https://pandas.pydata.org/pandas-docs/stable/development/contributing_codebase.html#testing-with-continuous-integration

as well as the tests in pandas.tests.window, and feel free to ask questions here.

@pep8speaks
Copy link

pep8speaks commented Jul 12, 2021 •
edited
Loading

Hello @benHeid! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-24 11:12:40 UTC

@benHeid
Copy link
Contributor Author

benHeid commented Jul 12, 2021

I added a parameterized test with different min_periods and windows.

jreback
jreback requested changes Jul 12, 2021
View reviewed changes
pandas/tests/window/test_groupby.py Outdated
@@ -275,8 +273,8 @@ def test_groupby_rolling_center_on(self):
)
result = (
df.groupby("gb")
.rolling(6, on="Date", center=True, min_periods=1)
.value.mean()
.rolling(6, on="Date", center=True, min_periods=1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you not change unrelated things

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls don't apply your settings otherwise this will not pass pre-commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I removed the autoformatted changes..

@benHeid benHeid requested a review from jreback July 16, 2021 07:08
@jreback
Copy link
Contributor

jreback commented Jul 16, 2021

there are still some precommit issues here

@benHeid
Copy link
Contributor Author

benHeid commented Jul 17, 2021

@github-actions pre-commit

@jreback jreback added this to the 1.4 milestone Jul 23, 2021
@jreback
Copy link
Contributor

jreback commented Jul 23, 2021

can you add a release note in 1.4 bug fix section (under rolling)

mroeschke
mroeschke reviewed Jul 24, 2021
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks. Just needs a whatnew note.

@jreback jreback merged commit f72c19b into pandas-dev:master Jul 28, 2021
@jreback
Copy link
Contributor

jreback commented Jul 28, 2021

thanks @benHeid

CGe0516 pushed a commit to CGe0516/pandas that referenced this pull request Jul 29, 2021
@benHeid @CGe0516
BUG: Mitigate division with zero in roll_var (pandas-dev#42459)
9f87934
feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021
@benHeid @feefladder
BUG: Mitigate division with zero in roll_var (pandas-dev#42459)
6810381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Window rolling, ewma, expanding
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.

BUG: Grouped Rolling Var and Std does not work with closed="left"
5 participants
@benHeid @jreback @rhshadrach @pep8speaks @mroeschke