Skip to content

BUG: return NaN from move_std and move_var when ddof >= count - #613

Open
raashish1601 wants to merge 1 commit into
pydata:masterfrom
raashish1601:fix/move-var-ddof
Open

raashish1601 wants to merge 1 commit into
pydata:masterfrom
raashish1601:fix/move-var-ddof

Conversation

@raashish1601

Copy link
Copy Markdown

move_std and move_var don't check that the number of values in the window is larger than ddof. When it isn't, they divide by zero or by a negative number, so you get inf, -0.0, or a negative variance:

>>> a = np.array([1.0, 2.0, 4.0, 7.0, 3.0])
>>> bn.move_var(a, window=3, min_count=1, ddof=2)
array([-0.        ,         inf,  4.66666667, 12.66666667,  8.66666667])
>>> bn.move_var(a, window=3, ddof=4)
array([         nan,          nan,  -4.66666667, -12.66666667,  -8.66666667])
>>> bn.slow.move_var(a, window=3, min_count=1, ddof=2)
array([        nan,         nan,  4.66666667, 12.66666667,  8.66666667])

nanvar/nanstd already return NaN when count <= ddof, and so does the slow reference (np.nanvar). This adds the same check to both the float and the integer code paths of the moving functions.

Added test_move_std_var_ddof_ge_count, which compares against bn.slow for float32/float64/int32/int64 and ddof from 0 up to window + 1. For integer input it compares against the float result, because np.var gives inf there while bn.nanvar gives NaN. The test fails on master and passes with this change. I ran the full test suite (216 passed, 1 xfailed) after building in a python:3.12 Docker image, plus ruff format --check and ruff check.

@raashish1601

Copy link
Copy Markdown
Author

The pre-commit.ci failure is zizmor flagging .github/workflows/ci.yml (self-repository syntax). master fails the same way, so it's unrelated to this change.

When the number of values in the window was not larger than ddof, the
moving variance divided by zero or a negative number, giving inf, -0.0
or negative variances. Return NaN instead, like nanvar and the slow
reference implementation.
@neutrinoceros

Copy link
Copy Markdown
Collaborator

Thanks for finding and fixing this.
Just running your trivially correct tests againt master shows that they correctly capture regressions. However I need to spend a couple minutes making sure I understand the problem and your solution well enough to then maintain it for ever. I'll try to make it happen before our upcoming release.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants