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

Activity

1. seberg

   added

   07 - Deprecation

   on Nov 4, 2021
2. seberg

   added this to the 1.22.0 release milestone on Nov 4, 2021
3. seberg

   self-assigned this

   on Nov 4, 2021
4. charris

   closed this as completedin #20327on Nov 16, 2021
5. rgommers commented on Nov 21, 2021

   

   rgommers

   on Nov 21, 2021

   Member

   More actions

   For completeness, this was mention on the mailing list in this thread: https://mail.python.org/archives/list/[email protected]/thread/XVEGVDMTG5C2TTP5DEFUDUJDCM363FEB/
6. jolynch commented on Jan 11, 2022

   

   jolynch

   on Jan 11, 2022

   More actions

   @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?
7. seberg commented on Jan 11, 2022

   

   seberg

   on Jan 11, 2022

   MemberAuthor

   More actions

   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?
8. BvB93 commented on Jan 11, 2022

   

   BvB93

   on Jan 11, 2022

   Last edited by BvB93

   Member

   More actions

   @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.
9. jolynch commented on Jan 11, 2022

   

   jolynch

   on Jan 11, 2022

   More actions

   @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.
10. jolynch commented on Jan 14, 2022

    

    jolynch

    on Jan 14, 2022

    More actions

    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?
11. seberg commented on Jan 16, 2022

    

    seberg

    on Jan 16, 2022

    MemberAuthor

    More actions

    @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.
12. BvB93 commented on Jan 17, 2022

    

    BvB93

    on Jan 17, 2022

    Last edited by BvB93

    Member

    More actions

    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.