ENH: Added nan_policy to circ functions in `stats` #10710

aburrell · 2019-08-23T13:24:26Z

Adds nan_policy as an optional keyword argument into the circular stat functions: stats.circmean, stats.circstd, and stats.circvar. NaN handling for all of these routines happens within stats.morestats._circfuncs_common.

What does this implement/fix?

This makes the circular statistical functions NaN tolerant, using the same method (user specified NaN policy of propagation, omission, or raising an error) as other functions within stats.

Additional information

Here is an example:

import numpy as np
from scipy import stats

stats.circmean([0.1, 2*np.pi+0.2, 6*np.pi+0.3])
stats.circmean([0.1, 2*np.pi+0.2, 6*np.pi+0.3, np.nan], nan_policy='omit')

Both return 0.2

- Added nan_policy as an optional keyword argument to circvar, circstd, circmean, and _circfuncs_common. - Updated docstrings for altered functions. - Added comments to _circfuncs_common. - Following the example of median_test, added NaN handling to samples in _circfuncs_common.

Added unit tests for the new optional keyword argument in the circular functions.

Added name and description of contribution to THANKS.txt

tylerjereddy

See brief associated mailing list discussion with @chrisb83 here: https://mail.python.org/pipermail/scipy-dev/2019-August/023684.html

scipy/stats/tests/test_morestats.py

rgommers

Hi @aburrell, thanks for this. Adding nan_policy is a good idea and this PR largely looks good. There's a couple of changes needed though.

scipy/stats/morestats.py

At the request of a reviewer, changed the general optional keyword arguements to a specific nan_policy keyword arguement.

Since the nan_policy keyword is now a parameter, we don't need to test for unknown keyword arguement input.

The nan_policy option 'omit' would not work when the axis keyword was not None. Fixed this by: - having circfuncs_common return separate angle arrays for the sine and cosine sums. - masking NaN the sine and cosine arrays with angles whose sine or cosine, respectively, equal zero.

Added the `pytest.mark.parametrize` decorator to appropriate unit tests in the TEstCircFuncs class.

Added unit tests for each of the different nan_policy flags with arrays of data that contain NaN. Tests behavior when axis flag is None, 0, and 1.

Fixed bugs that occured when using arrays containing np.nan and outputting the circular statistics along a specified axis. - Changed _circfuncs_common to also output the NaN mask. - Added check to see if all the data along a specified axis are NaN. - Changed method of calculating mean in circstd and circvar if the data is masked.

rgommers · 2019-08-27T19:12:39Z

The failures are all because of warnings:

RuntimeWarning: invalid value encountered in less

In CI we change the default behavior for warnings to result in test errors, because we want to prevent introducing new warnings when running the SciPy test suite. You should also be seeing warnings locally. If you silence the warnings by putting the relevant lines within a with np.errstate(invalid="ignore"): context manager, then CI should pass. There's a lot of examples of that in the stats tests already.

Changed processing order to prevent RuntimeWarnings in circmean, circvar, and circstd. - Changed masking in circmean to not try and evaluate inequality with NaN - Fixed logic in circstd and circvar masking to avoid division by zero

Fixed several instances of inappropriate whitespace: - No whitespace around an operator. - Too much whitespace around an operator. - Wrong number of spaces causing bad indentation.

aburrell · 2019-08-28T19:43:26Z

It wasn't particularly difficult to fix the code to avoid introducing new RuntimeWarnings. I think it's ready for another review now.

scipy/stats/morestats.py

Implemented reviewer suggestions to improve the clarity of the code: - Used np.asarray to reduce instances of dimension checking. - Replaced np.copyto with masking reassignment. - Added comments to clarify the axis-checking portion of the circmean routine

Simplified the NaN axis evaluation in circmean by using shape instead of sum.

Implemented reviewer simplification suggestions and fixed a bug introduced in the last commit: - Fixed bug in circmean that was triggered when axis=None. - Moved sin/cos calculation to _circfuncs_common. - Replaced copyto with a simple mask in _circfuncs_common.

Fixed bug that output a single NaN when an array was expected in circmean.

Added a unit test for a multi-dimensional all NaN input when the 'omit' flag is used, ensuring that the output is also an array of the expected dimensions.

Removed the backslash within brackets that was causing pyflakes to fail in for python 3.6.

rgommers · 2019-10-02T14:15:13Z

All my comments were addressed and this now looks good to me. @pv's comments also seem addressed, but I don't have the bandwidth to re-review everything in detail right now, so I'll hold off on doing the actual merge (will do so before 1.4.0 if no one else beats me to it).

rgommers · 2019-10-20T11:52:18Z

All looks good, time to merge this. Thanks @aburrell!

aburrell · 2019-10-21T13:38:55Z

Thank you @rgommers, @tylerjereddy, and @pv! This was a good experience contributing code, and I appreciate the time you all put in to improve my contribution to scipy.

rgommers · 2019-10-21T13:57:58Z

Thanks @aburrell, glad it was a good experience! Will try to get the next PR merged faster:)

aburrell added 3 commits August 23, 2019 09:10

TST: nan_policy unit tests for circ functions

a332b06

Added unit tests for the new optional keyword argument in the circular functions.

DOC: added name to THANKS.txt

808ad76

Added name and description of contribution to THANKS.txt

tylerjereddy added scipy.stats enhancement A new feature or improvement labels Aug 23, 2019

tylerjereddy reviewed Aug 23, 2019

View reviewed changes

scipy/stats/tests/test_morestats.py Outdated Show resolved Hide resolved

rgommers requested changes Aug 23, 2019

View reviewed changes

scipy/stats/morestats.py Outdated Show resolved Hide resolved

scipy/stats/morestats.py Outdated Show resolved Hide resolved

scipy/stats/morestats.py Outdated Show resolved Hide resolved

aburrell and others added 7 commits August 26, 2019 07:27

ENH: changed to nan_policy keyword arg

dab3e03

At the request of a reviewer, changed the general optional keyword arguements to a specific nan_policy keyword arguement.

TST: removed unknown keyword unit test

a1a0a4b

Since the nan_policy keyword is now a parameter, we don't need to test for unknown keyword arguement input.

TST: parametrized TestCircFuncs

1ecb5b3

Added the `pytest.mark.parametrize` decorator to appropriate unit tests in the TEstCircFuncs class.

TST: added TestCircFuncs unit tests for arrays with nan_policy

9580431

Added unit tests for each of the different nan_policy flags with arrays of data that contain NaN. Tests behavior when axis flag is None, 0, and 1.

Merge branch 'master' into circfunc_nan_policy

3564d30

aburrell added 2 commits August 28, 2019 13:29

BUG: fixed RuntimeWarnings in circ functions

48066e6

Changed processing order to prevent RuntimeWarnings in circmean, circvar, and circstd. - Changed masking in circmean to not try and evaluate inequality with NaN - Fixed logic in circstd and circvar masking to avoid division by zero

BUG: fixed inappropriate whitespace

d4ab03c

Fixed several instances of inappropriate whitespace: - No whitespace around an operator. - Too much whitespace around an operator. - Wrong number of spaces causing bad indentation.

rgommers added this to the 1.4.0 milestone Aug 28, 2019

pv reviewed Sep 8, 2019

View reviewed changes

aburrell added 6 commits September 11, 2019 15:08

ENH: simplified the NaN axis evaluation

0251ff5

Simplified the NaN axis evaluation in circmean by using shape instead of sum.

BUG: all NaN output arrays in circmean

8365265

Fixed bug that output a single NaN when an array was expected in circmean.

TST: tested all NaN array output with 'omit' in circfuncs

f8c097c

Added a unit test for a multi-dimensional all NaN input when the 'omit' flag is used, ensuring that the output is also an array of the expected dimensions.

BUG: style fix for circmean

2028c7a

Removed the backslash within brackets that was causing pyflakes to fail in for python 3.6.

aburrell requested a review from rgommers September 16, 2019 13:37

rgommers approved these changes Oct 2, 2019

View reviewed changes

rgommers merged commit 2c7c004 into scipy:master Oct 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Added nan_policy to circ functions in `stats` #10710

ENH: Added nan_policy to circ functions in `stats` #10710

aburrell commented Aug 23, 2019 •

edited by tylerjereddy

tylerjereddy left a comment

rgommers left a comment

rgommers commented Aug 27, 2019

aburrell commented Aug 28, 2019

rgommers commented Oct 2, 2019

rgommers commented Oct 20, 2019

aburrell commented Oct 21, 2019

rgommers commented Oct 21, 2019

ENH: Added nan_policy to circ functions in stats #10710

ENH: Added nan_policy to circ functions in stats #10710

Conversation

aburrell commented Aug 23, 2019 • edited by tylerjereddy

What does this implement/fix?

Additional information

tylerjereddy left a comment

Choose a reason for hiding this comment

rgommers left a comment

Choose a reason for hiding this comment

rgommers commented Aug 27, 2019

aburrell commented Aug 28, 2019

rgommers commented Oct 2, 2019

rgommers commented Oct 20, 2019

aburrell commented Oct 21, 2019

rgommers commented Oct 21, 2019

ENH: Added nan_policy to circ functions in `stats` #10710

ENH: Added nan_policy to circ functions in `stats` #10710

aburrell commented Aug 23, 2019 •

edited by tylerjereddy