BUG: deprecation for ignored corrcoef args

[matthew-brett]

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.

When the bias argument goes away, we will need to make the
allow_masked argument to ma.corrcoef into a keyword-only argument; warn
about this change.

[charris]

This also needs a note iin doc/release/1.10.0-notes.rst under Deprecations.

[charris]

Tests are failing ERROR: Failure: ImportError (No module named warnutils).

[matthew-brett]

Oops - should be fixed now.

[charris]

I think we can simplify this further if we add a NoValue class to numpy/__init__.py. Something like

class NoValue(object):
    """Special keyword value.

    This class may be used as the default value assigned to a
    deprecated keyword in order to check if it has been given a user
    defined value.

    """
    pass

Then it can be used as the default value of bias and ddof and checked like so

if bias is not NoValue:
    <warning>

The argument count is then handled by python itself.

[matthew-brett]

Right - sort of like _DefaultArg. I went for the args, kwargs things in part to remove bias and ddof from the function signature, but perhaps it isn't worth the hassle.

[charris]

And then perhaps a single warning message, maybe bias and ddof have no affect and are deprecated.

[matthew-brett]

Do we really want NoValue in the top-level numpy namespace?

[matthew-brett]

Also - that won't work for deprecating allow_masked as a positional argument - I don't think.

[matthew-brett]

I implemented your suggestion - do you like how it looks?

[charris]

Yeah, it won't work for positional arguments. The ma.corrcoef looks OK, though.

ma.corrcoef(x, y=None, rowvar=True, bias=False, allow_masked=True, ddof=None)

We won't be able to remove the arguments anytime soon ;) But using *arg and **kwargs doesn't remove the arguments either, it just makes them harder to find.

[matthew-brett]

Current version OK with you then?

[charris]

Two blank lines ;)

[njsmith]

Can we not export this publicly? Just call it _NoValue or something? We
have enough public clutter as it is, and this adds zero value.
On Mar 14, 2015 9:15 PM, "Charles Harris" notifications@github.com wrote:

In numpy/init.py
#5683 (comment):

@@ -132,6 +132,15 @@ class VisibleDeprecationWarning(UserWarning):
pass

+class NoValue:

• """Special keyword value.

• This class may be used as the default value assigned to a
• deprecated keyword in order to check if it has been given a user
• defined value.
• """
• pass

Two blank lines ;)

—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/5683/files#r26444409.

[charris]

Hmm, I guess it doesn't need to be public, so _NoValue would work.

[charris]

Should probably keep the documentation for both of these, but just say they have no affect and are deprecated. Like so

    bias :
        .. deprecated:: 1.10.0
        Has no affect, not used.

[charris]

Make that

    bias : NoValue, optional
        .. deprecated:: 1.10.0
        Has no affect, do not use.

[charris]

Almost there I think.

[matthew-brett]

Can I leave it to you guys to decide on NoValue vs _NoValue?

[charris]

Yes. @rgommers Probably need a third opinion for that.

[njsmith]

There's just zero reason to expose it: the requirements are that it be a
value which (a) we can check for, (b) users won't pass in. Being public is
irrelevant to (a), and actually bad for (b). And it's an implementation
detail anyway.
On Mar 14, 2015 9:54 PM, "Charles Harris" notifications@github.com wrote:

Yes. @rgommers https://github.com/rgommers Probably need a third
opinion for that.

—
Reply to this email directly or view it on GitHub
#5683 (comment).

[charris]

NVM, I think using _NoValue is the way to go.

[matthew-brett]

Do you agree we need args, kwargs in ma.corrcoef for checking for positional argument to allow_masked? If so, I'll rebase ready for merge.

[charris]

@matthew-brett I don't see why they are needed, all you need to know is if the argument is used. If allow_masked is used as a positional argument, it needs to be preceded by a positional value for bias and you will detect that. What is your thinking?

[matthew-brett]

I believe we need to

a) give two warnings when bias and allow_masked get used as positional arguments together and
b) give a new warning about allow_masked if the user first calls corrcoef with bias (and gets a warning) and then calls corrcoef with bias AND allow_masked.

Did I miss how that can be done without using args, kwargs?

[charris]

The way I see it is that the user will always get a warning if they use bias or ddof in any fashion, positional or otherwise. If they use bias as a keyword argument, no positional arguments can follow. If they use bias as a positional argument, they will need to use allow_masked as a keyward argument to remove the bias value and avoid the warning. In both cases the fix will be to use allow_masked as a keyword argument, which is what we want. Could add a note to docstring that the remaining arguments should all be treated as keyword arguments. Current practice in numpy/scipy is to avoid using *args and **kwargs, so there is also that.

I need to head off to bed now. To be continued tomorrow...

[matthew-brett]

OK - you are saying that the user will in any case infer that allow_masked should be keyword only. Or, put another way, the only way to make the warnings go away is to use allow_masked as a keyword.

I don't feel strongly, happy to make the change. I currently test for the warnings in situations a, b above, but I can remove those tests.

[charris]

Maybe something like Because bias is deprecated, this argument needs to be treated as keyword only to avoid a warning.

[charris]

@matthew-brett With that change I think this would be ready.

[charris]

Merged. Thanks Matthew.