Add ddof argument to np.nanvar and np.nanstd (fixes #6611)#10540
Add ddof argument to np.nanvar and np.nanstd (fixes #6611)#10540furperson wants to merge 9 commits into
Conversation
|
Hi, could someone please take a look at this when you get a chance? All CI checks are passing. Thanks! |
|
Hi @swap357 , could you please take a look at this PR when you have a moment? All CI checks are passing. Thanks! |
swap357
left a comment
There was a problem hiding this comment.
Thank you for the PR, @furperson! Did a first pass review and left comments. Since this is changing overload signature, could you document points about added support and limitations for np.nanvar and np.nanstd on - https://numba.readthedocs.io/en/stable/extending/overloading-guide.html.
|
|
||
| @overload(np.nanvar) | ||
| def np_nanvar(a): | ||
| def np_nanvar(a, axis=None, dtype=None, out=None, ddof=0): |
There was a problem hiding this comment.
The standard practice in Numba is to keep signature minimal. If the scope of the PR is ddof, can we keep signature to only that ? errors for the other args can handled by dispatcher -
With argument(s): '(array(float64, 1d, C), axis=int64)':
Rejected as the implementation raised a specific error:
TypingError: got an unexpected keyword argument 'axis'
|
|
||
| @overload(np.nanstd) | ||
| def np_nanstd(a): | ||
| def np_nanstd(a, axis=None, dtype=None, out=None, ddof=0): |
There was a problem hiding this comment.
same as above, keep only needed changes to signature
| def nanvar_impl(a, axis=None, dtype=None, out=None, ddof=0): | ||
| # For complex input NumPy uses abs(v - m)**2 (always real) | ||
| m = np.nanmean(a) | ||
| n = 0 |
There was a problem hiding this comment.
Please try and keep the existing implementation and naming for variables, if it's not modifying logic. It helps to understand what's changing. Synthetic refactor can be a separate PR if needed.
| def test_nanvar_ddof(self): | ||
| self.check_reduction_basic(array_nanvar_ddof0, prec='double') | ||
| self.check_reduction_basic(array_nanvar_ddof1, prec='double') | ||
|
|
There was a problem hiding this comment.
Complex branch not being tested
|
Hi @swap357, I've updated the signature to minimal Previously the full signature |
Hi @furperson, Thank you for the changes. Apologies, but I now think my earlier review comment wasn't entirely correct. I went through the testing code and codebase to check for overloads with less params than their corresponding numpy signature. I did find some, like Line 2977 in 3c9c807 It escapes the test because it does mention supported args upto ddof and omits the rest of the unsupported ones after it (fweights, aweights)For this PR, I think it would be correct to revert to what you had and write all args upto ddof.
For the unsupported ones, guarding it with error is good. There is precedent at Lines 1037 to 1039 in 3c9c807 Again, thank you for the patience! |
Fixes #6611
Added
ddof(Delta Degrees of Freedom) parameter tonp.nanvarandnp.nanstdin nopython mode, matching NumPy behaviour.Changes:
np.nanvar(a, ddof=0): default ddof=0 preserves backward compatibilitynp.nanstd(a, ddof=0): passes ddof through to nanvarconsistent with NumPy)
to return a real result, consistent with NumPy
Tests added in numba/tests/test_array_reductions.py: