Skip to content

DEP: Rename quantile/percentile interpolation kwarg to method #20306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
seberg opened this issue Nov 4, 2021 · 8 comments · Fixed by #20327
Closed

DEP: Rename quantile/percentile interpolation kwarg to method #20306

seberg opened this issue Nov 4, 2021 · 8 comments · Fixed by #20327
Assignees

Comments

@seberg
Copy link
Member

seberg commented Nov 4, 2021

It seems like a good idea to rename the quantile/percentile "interpolation" kwarg to method. This is because:

  • Method seems more descriptive, "interpolation" doesn't actually capture all of the complexity (anymore)
  • The old versions (except the default) were not really described in literature anyway. So this nudges users to switch to a different value.
@rgommers
Copy link
Member

For completeness, this was mention on the mailing list in this thread: https://mail.python.org/archives/list/numpy-discussion@python.org/thread/XVEGVDMTG5C2TTP5DEFUDUJDCM363FEB/

@jolynch
Copy link

jolynch commented Jan 11, 2022

@seberg I believe that 8437663e broke backwards compatibility for mypy type checking of the np.percentile method even though the underlying method still contains the argument. A minimal reproduction is

import numpy as np

def test_method() -> float:
    return np.percentile([1, 2, 4, 5], [50], method="nearest")

def test_interpolation() -> float:
    return np.percentile([1, 2, 4, 5], [50], interpolation="nearest")

print("interpolation=", test_interpolation())
print("method=", test_method())

On 1.22.0 the code works with both method and interpolation but type checking fails

$ python3 -m venv venv-1.22 && venv-1.22/bin/pip install mypy numpy==1.22.0
...
$ venv-1.22/bin/python test.py                                                                                                                                                                                      
test.py:7: DeprecationWarning: the `interpolation=` argument to percentile was renamed to `method=`, which has additional options.
Users of the modes 'nearest', 'lower', 'higher', or 'midpoint' are encouraged to review the method they. (Deprecated NumPy 1.22)
  return np.percentile([1, 2, 4, 5], [50], interpolation="nearest")
interpolation= [4]
method= [4]

# BUT we fail mypy type checking because interpolation was removed 
$ venv-1.22/bin/mypy test.py
test.py:4: error: Incompatible return value type (got "ndarray[Any, dtype[floating[Any]]]", expected "float")
test.py:7: error: No overload variant of "percentile" matches argument types "List[int]", "List[int]", "str"
test.py:7: note: Possible overload variants:
...

Then on 1.21.5 typechecking is ok on interpolation but obviously we can't use method because it doesn't exist

$ python3 -m venv venv-1.21 && venv-1.22/bin/pip install mypy numpy==1.21.5
...
# Expectedly we fail when passing method
$ venv-1.21/bin/python test.py 
interpolation= [4]
Traceback (most recent call last):
  File "test.py", line 10, in <module>
    print("method=", test_method())
  File "test.py", line 4, in test_method
    return np.percentile([1, 2, 4, 5], [50], method="nearest")
  File "<__array_function__ internals>", line 4, in percentile
TypeError: _percentile_dispatcher() got an unexpected keyword argument 'method'
# Mypy also fails
$ venv-1.21/bin/mypy test.py
test.py:4: error: Unexpected keyword argument "method" for "percentile"
venv-1.21/lib/python3.8/site-packages/numpy/lib/function_base.pyi:50: note: "percentile" defined here
Found 1 error in 1 file (checked 1 source file)

Perhaps would it be best to not replace interpolation with method in the overloads and just add the method argument to the overload signatures? Then folks could use either the old argument or the new one and mypy would be happy with either?

@seberg
Copy link
Member Author

seberg commented Jan 11, 2022

Right now I think we alwas "opted in" mypy users to the future behaviour in such API changes. @BvB93 what do you think? Is there even a way to mark an overload as deprecated in the stubs?

@BvB93
Copy link
Member

BvB93 commented Jan 11, 2022

@seberg I believe that 8437663 broke backwards compatibility for mypy type checking of the np.percentile method

It's as much as a backwards incompatible change as any other deprecation. If you want to keep using the method kwarg and keep mypy happy I'd suggest to just put in a # type: ignore comment.

def test_interpolation() -> float:
-    return np.percentile([1, 2, 4, 5], [50], interpolation="nearest")
+    return np.percentile([1, 2, 4, 5], [50], interpolation="nearest")  # type: ignore

EDIT: Mixed up the interpolation and method kwarg.

@jolynch
Copy link

jolynch commented Jan 11, 2022

@BvB93 the difference is that the deprecation didn't remove the argument; the library function correctly has both arguments for backwards compatibility and simply warns you are using a deprecated argument. The type annotations renamed the parameter instead of adding a parameter which is not backwards compatible (as opposed to keeping both).

Your suggestion of using method instead of interpolation would require a >= pin in any library which depends on numpy. If you're a library owner who is using numpy the only safe choice is interpolation unless you require the functionality of method.

@jolynch
Copy link

jolynch commented Jan 14, 2022

If I raised a patch with interpolation added back to the function_base.pyi for backwards compatibility (and it can be removed when a major version removes interpolation from the base methods) would that be reasonable?

Alternatively I'm not really sure the value of deprecating interpolation is as it seems like a cheap compat shim to maintain? I understand the desire to align with R but numpy has established a perfectly fine interface that has worked for a long time here ... I don't see any inherent virtue in the method words coming from literature instead of numerous years of use in real software?

@seberg
Copy link
Member Author

seberg commented Jan 16, 2022

@jolynch I don't have a real opinion about it (about the typing; I am nowhere close to considering the alternative). I tried to see if the typing world voices a very clear opinion, but for now it didn't answer clearly ;). As of now, I feel whatever @BvB93 prefers, goes. (Unless maybe we have very clear "prior art" from elsewhere, or more clear opinions from typing specialists somewhere.) I do not understand that # type: ignore should be a major hassle.

@BvB93
Copy link
Member

BvB93 commented Jan 17, 2022

Up to this point we've always treated the annotations of deprecated functions/parameters as if they've been completed, i.e. the respective function/parameter is removed from the stub file (this might be worthwhile documenting somewhere though; xref #20836). Until some proper standardization is ever introduced for the typing deprecated functions/parameters I'd be in favor of this approach, as type checkers will start producing warnings, similar to the DeprecationWarnings issues during runtime.

I just realized the wrong kwarg-name was initially used in #20306 (comment), but the main point still stand: just as you can ignore warnings during runtime, so can you do the same while static type checking with # type: ignore comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants