-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: rolling mean and sum not numerical stable for all nan window #41106
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.
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
Conversation
…rking � Conflicts: � doc/source/whatsnew/v1.3.0.rst
] | ||
) | ||
|
||
expected = DataFrame( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind parameterizing over expected
and the slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could yes, but you make a pretty long parametrization, because most of exepected differs too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parametrized, got a bit better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM the CI failure look unrelated.
Can open up another issue if you have time to investigate if we need the nobs > 0
check elsewhere.
cc @mroeschke
on master. Sum should be the only other case we have the problem. As long as all nan should return 0 we can do it this way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking. Just one comment about the whatsnew otherwise LGTM
doc/source/whatsnew/v1.3.0.rst
Outdated
@@ -846,6 +846,7 @@ Groupby/resample/rolling | |||
- Bug in :meth:`GroupBy.cummin` and :meth:`GroupBy.cummax` incorrectly rounding integer values near the ``int64`` implementations bounds (:issue:`40767`) | |||
- Bug in :meth:`.GroupBy.rank` with nullable dtypes incorrectly raising ``TypeError`` (:issue:`41010`) | |||
- Bug in :meth:`.GroupBy.cummin` and :meth:`.GroupBy.cummax` computing wrong result with nullable data types too large to roundtrip when casting to float (:issue:`37493`) | |||
- Bug in :meth:`DataFrame.rolling` returning mean zero, respectively sum not zero for all ``NaN`` window with ``min_periods=0`` if calculation is not numerical stable (:issue:`41053`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add 2 separate lines for the mean bug and sum bug for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
thanks @phofl |
The problem is not numerical stable, so we are dividing -3e-18 / 0, which returns -inf running into line 177 setting to 0. Checking for nobs > 0 would solve these cases
cc @mroeschke