MRG: deprecate ddof / bias args to corrcoef #5675

matthew-brett · 2015-03-13T00:32:45Z

As discussed on the mailing list, the values for the bias and ddof
arguments to the corrcoef cancel in calculation, and are therefore
pointless and confusing.

Deprecate these arguments and document their pending removal.

This PR also includes a version of the warnings.catch_warnings decorator
that allows warnings to be tested for multiple times without error.

njsmith · 2015-03-13T00:42:17Z

You mean "context manager", not " decorator". Everything else looks good to
me (on a quick skim at least).
On Mar 12, 2015 5:32 PM, "Matthew Brett" notifications@github.com wrote:

As discussed on the mailing list, the values for the bias and ddof
arguments to the corrcoef cancel in calculation, and are therefore
pointless and confusing.

Deprecate these arguments and document their pending removal.

This PR also includes a version of the warnings.catch_warnings decorator

that allows warnings to be tested for multiple times without error.

You can view, comment on, or merge this pull request online at:

#5675
Commit Summary

ENH+TST: add decorator for testing warnings

BUG: deprecation for ignored corrcoef args

File Changes

M numpy/lib/function_base.py
https://github.com/numpy/numpy/pull/5675/files#diff-0 (45)

M numpy/lib/tests/test_function_base.py
https://github.com/numpy/numpy/pull/5675/files#diff-1 (31)

M numpy/ma/extras.py
https://github.com/numpy/numpy/pull/5675/files#diff-2 (50)

M numpy/ma/tests/test_extras.py
https://github.com/numpy/numpy/pull/5675/files#diff-3 (76)

M numpy/ma/tests/test_regression.py
https://github.com/numpy/numpy/pull/5675/files#diff-4 (15)

A numpy/testing/tests/test_warnutils.py
https://github.com/numpy/numpy/pull/5675/files#diff-5 (48)

A numpy/testing/warnutils.py
https://github.com/numpy/numpy/pull/5675/files#diff-6 (36)

Patch Links:

https://github.com/numpy/numpy/pull/5675.patch

https://github.com/numpy/numpy/pull/5675.diff

—
Reply to this email directly or view it on GitHub
#5675.

Warnings can be slippery, because, whenever a warning is triggered, Python adds a __warningregistry__ member to the *calling* module. This makes it impossible to retrigger the warning in this module, whatever you put in simplefilter. The `catch_warn_reset` decorator removes the __warningregistry__ member as the context manager exits, making it possible to retrigger the warning.

The `ddof` and `bias` arguments to `corrcoef` cancel in the math of the correlation coefficient, so there is no point in passing these values, and it is confusing to the user to see these in the docstring. Remove, deprecate, and test.

matthew-brett · 2015-03-13T00:49:05Z

Thanks - I fixed references to 'decorator'.

The main question that I have is what to do about the 'bias' argument to np.ma.extras.corrcoef. It is before a still-used argument allow_masked.

It might be prudent to plan to make allow_masked a keyword-only argument at the same time as the end of the deprecation period for bias, so intended inputs to bias don't get picked up by allow_masked. This would involve deprecating allow_masked as a positional argument.

jaimefrio · 2015-03-13T01:42:42Z

numpy/lib/function_base.py

+    arguments had no effect on the return values of the function and can be
+    safely ignored in this and previous versions of numpy.
+    """
+    if len(args) or kwargs.pop('bias', _DefaultArg) is not _DefaultArg:


We are opening the door to lots of nonsense input going through silently, e.g. extra keyword or positional arguments, bias and ddof being provided as both positional and keyword simultaneously... May not be worth adding all those checks for something we actually want to deprecate, but...

Checking for simultaneous positional and keyword does seem too much for arguments that don't have any effect on the function. We are already warning about their use.

It's easy to check for the length of args and kwargs after parsing though.

"allow_masked" follows "bias" argument, but "bias" will go away at some point. Intend that we will make "allow_masked" keyword only at the same time that we remove "bias".

Raise errors for the wrong number of positional arguments or incorrect keyword arguments in corrcoef routines.

matthew-brett · 2015-03-13T22:05:04Z

I added a deprecation warning for allow_masked as positional argument as above.

I added checks for number of positional args and unexpected keyword args.

charris · 2015-03-14T18:20:05Z

numpy/ma/extras.py

@@ -48,7 +48,7 @@
 from numpy import ndarray, array as nxarray
 import numpy.core.umath as umath
 from numpy.lib.index_tricks import AxisConcatenator
-from numpy.linalg import lstsq
+from numpy.lib.function_base import _CORRCOEF_MSG_FMT, _DefaultArg


Just repeat the definition here. This import complicates the code, obscures the value of _CORRCOEF_MSG_FMT, and introduces a dependency between modules. The format is temporary anyway. No reason not to also put the private _DefaultArg here also, and for the same reasons.

Willco.

Any suggestions for a good place for _DefaultArg?

Since its use is entirely local, it might be better to just define it in each module in which it is used. It is going to be temporary, at least in the numpy sense of temporary. Otherwise, I might suggest numpy/__init__.py, which already has a couple of global classes. In the latter case a more suggestive name might help.

Rather than importing the small code support fragments from function_base.py, replicate them to make the code easier to read.

Note that this is the Pearson product-moment correlation. Note the pending change of allow_masked to keyword-only.

Add ability to add default modules to catch_warn_reset classs, by inheritance.

charris · 2015-03-14T19:03:17Z

Both the deprecation and context manager should be mentioned in the release notes. Apropos the context manager, a separate PR might be best. A name closer to the standard catch_warnings would also be good something like catch_and_reset_warnings. I also wonder if it wouldn't be possible to just integrate it into assert_warns?

charris · 2015-03-14T19:10:13Z

Or maybe catch_and_clear_warnings for added alliteration.

Use inheritance of catch_warn_reset for slightly cleaner context managers.

matthew-brett · 2015-03-14T19:17:59Z

No problem for separate PR, and renaming of context manager.

Adding to assert_warns would need a new 'modules' argument to assert_warns, at least the way I have written it.

Sckit-image go for a more magic and more nuclear solution, always clearing out all recorded previous warnings for modules in the call stack:

https://github.com/scikit-image/scikit-image/blob/master/skimage/_shared/_warnings.py

charris · 2015-03-14T19:23:45Z

Google also turned up that nuclear option. So there is no easy way to know what module a warning was raised in when it is caught?

matthew-brett · 2015-03-14T19:25:41Z

I don't know of any way to find out what module the warning was raised in, from the warning itself...

charris · 2015-03-14T19:29:47Z

Thinking about it, both the format and class could be local to the function, that way everything could be deleted at one go when it comes to that.

matthew-brett · 2015-03-14T19:32:51Z

My worry about putting the class definition and format string in the funciton was that it made the meat of the function harder to read. I can imagine that it might be distracting to see this little piece of uncecessarily repeated work inside the function.

charris · 2015-03-14T19:34:23Z

Good point. OTOH, if it is outside the function one wonders where else it is used.

Move the format strings into the function, as they are only used in the function.

matthew-brett · 2015-03-14T19:43:39Z

I was hoping that wondering where else it is used it more typical of the keen-eyed maintainer than the casual developer or user.

I've moved just the string into the function - how about that?

charris · 2015-03-14T19:53:43Z

OK. Too bad we can't just use None. Hmm..., if the default value of ddof is made 0, I think None would be OK.

matthew-brett · 2015-03-14T20:00:08Z

My worry with None is that it's not possible to tell whether the user passed it, thinking the argument still existed.

charris · 2015-03-14T20:09:32Z

How about just check for the key, then pop it if present and issue the warning?

matthew-brett · 2015-03-14T21:56:21Z

Your nit-pick is my command.

If this is all good I'll rebase into two PRs.

charris · 2015-03-14T22:13:20Z

numpy/lib/function_base.py

+        warnings.warn(fmt.format('ddof'), DeprecationWarning)
+    if len(kwargs):
+        raise TypeError(
+            "corrcoef got an unexpected keyword argument '{0}'".format(


Note that you can break this line

raise TypeError("corrcoef got an unexpected keyword " "argument '{0}'".format(list(kwargs)[0])

charris · 2015-03-14T22:14:28Z

LGTM, go for it. I tossed in another nitpick...

matthew-brett · 2015-03-14T22:16:13Z

OK to rename context manager to catch_clear_warnings ?

charris · 2015-03-14T22:29:56Z

I'd probably leave the _and_ in there.

charris · 2015-03-14T22:38:59Z

Although I confess to being a bit unsure why the context manager is needed. Does this fix a problem when the tests are run in interactive mode?

matthew-brett · 2015-03-14T22:42:11Z

I moved the context manager into numpy.testing.utils and added more docstring - does the docstring help explain the problem the context manager is trying to solve?

matthew-brett · 2015-03-14T23:01:52Z

OK to go ahead with 2 PRs from this state?

charris · 2015-03-14T23:02:14Z

numpy/testing/tests/test_utils.py

@@ -1,10 +1,16 @@
 from __future__ import division, absolute_import, print_function

 import warnings
+from warnings import warn, simplefilter


We usually just import warnings and use it in the warnings.warn form.

charris · 2015-03-14T23:16:39Z

Sure. I won't guarantee no more comments, but I think this is getting close.

matthew-brett · 2015-03-15T00:19:29Z

Maybe the context manager should better be called catch_all_warnings_mods or something else to reflect the fact that it only resets the warnings registry inside the context manager.

matthew-brett · 2015-03-15T00:50:54Z

Or clear_and_catch_warnings

matthew-brett · 2015-03-15T01:17:30Z

OK - I think this is ready now - will split into separate PRs soon.

matthew-brett · 2015-03-15T01:35:36Z

Context manager PR here : #5682

matthew-brett · 2015-03-15T02:34:06Z

Deprecation stuff here : #5683

charris · 2015-03-15T02:40:09Z

OK, I'll close this now.

matthew-brett added 2 commits March 12, 2015 17:44

BUG: deprecation for ignored corrcoef args

e88b608

The `ddof` and `bias` arguments to `corrcoef` cancel in the math of the correlation coefficient, so there is no point in passing these values, and it is confusing to the user to see these in the docstring. Remove, deprecate, and test.

matthew-brett force-pushed the deprecate-corrcoef-ddof branch from 40bf776 to e88b608 Compare March 13, 2015 00:45

jaimefrio reviewed Mar 13, 2015
View reviewed changes

matthew-brett added 2 commits March 13, 2015 14:46

ENH: make ma.corrcoef allow_masked keyword-only

e6fb365

"allow_masked" follows "bias" argument, but "bias" will go away at some point. Intend that we will make "allow_masked" keyword only at the same time that we remove "bias".

ENH: raise errors for wrong arguments in corrcoef

3c5975b

Raise errors for the wrong number of positional arguments or incorrect keyword arguments in corrcoef routines.

charris reviewed Mar 14, 2015
View reviewed changes

matthew-brett added 3 commits March 14, 2015 11:36

ENH: replicate code support fragments for clarity

559ebd2

Rather than importing the small code support fragments from function_base.py, replicate them to make the code easier to read.

DOC: update docstring for ma.extras.corrcoef

523efdb

Note that this is the Pearson product-moment correlation. Note the pending change of allow_masked to keyword-only.

ENH: allow default modules for catch_warn_reset

debcfb9

Add ability to add default modules to catch_warn_reset classs, by inheritance.

charris added component: numpy.lib component: numpy.ma masked arrays 07 - Deprecation labels Mar 14, 2015

ENH: use new catch_warn_reset inheritance

5dc9d6c

Use inheritance of catch_warn_reset for slightly cleaner context managers.

ENH: move format strings into body of function

275c20f

Move the format strings into the function, as they are only used in the function.

matthew-brett added 2 commits March 14, 2015 14:53

ENH: add line break in corrcoef error message

b4e40ef

ENH: rename format argument

bdfd37b

charris reviewed Mar 14, 2015
View reviewed changes

ENH: rename catch_warn_reset to catch_clear_warnings

00aae51

ENH: reformat another warning string

33857d2

ENH: move warnings context manager, rename

18549b2

matthew-brett added 2 commits March 14, 2015 15:47

DOC: fix trailing rename

7634737

ENH: reformat multiple imports

cfc77e0

charris reviewed Mar 14, 2015
View reviewed changes

matthew-brett added 2 commits March 14, 2015 16:43

ENH: use 'warnings' module instead of importing from

022017d

DOC: fill in docstring, make args, kwargs explicit

c1f5e99

matthew-brett force-pushed the deprecate-corrcoef-ddof branch from d81d18a to c1f5e99 Compare March 14, 2015 23:44

ENH: rename warnings context manager

e863842

charris closed this Mar 15, 2015

MRG: deprecate ddof / bias args to corrcoef #5675

MRG: deprecate ddof / bias args to corrcoef #5675

Conversation

matthew-brett commented Mar 13, 2015

njsmith commented Mar 13, 2015

that allows warnings to be tested for multiple times without error.

matthew-brett commented Mar 13, 2015

jaimefrio Mar 13, 2015

Choose a reason for hiding this comment

matthew-brett Mar 13, 2015

Choose a reason for hiding this comment

matthew-brett commented Mar 13, 2015

charris Mar 14, 2015

Choose a reason for hiding this comment

matthew-brett Mar 14, 2015

Choose a reason for hiding this comment

charris Mar 14, 2015

Choose a reason for hiding this comment

charris commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris Mar 14, 2015

Choose a reason for hiding this comment

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris commented Mar 14, 2015

charris commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

matthew-brett commented Mar 14, 2015

charris Mar 14, 2015

Choose a reason for hiding this comment

charris commented Mar 14, 2015

matthew-brett commented Mar 15, 2015

matthew-brett commented Mar 15, 2015

matthew-brett commented Mar 15, 2015

matthew-brett commented Mar 15, 2015

matthew-brett commented Mar 15, 2015

charris commented Mar 15, 2015