Skip to content

TYP: remove ignores in refine_percentiles #42389

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 7, 2021
Merged

TYP: remove ignores in refine_percentiles #42389

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1cf6df
TYP: remove ignores in refine_percentiles
simonjayhawkins Jul 5, 2021
3e664e8
Merge remote-tracking branch 'upstream/master' into ignore-2
simonjayhawkins Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions pandas/core/describe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
)
from typing import (
TYPE_CHECKING,
Any,
Callable,
Sequence,
cast,
Expand Down Expand Up @@ -50,7 +51,7 @@ def describe_ndframe(
include: str | Sequence[str] | None,
exclude: str | Sequence[str] | None,
datetime_is_numeric: bool,
percentiles: Sequence[float] | None,
percentiles: Sequence[float] | np.ndarray | None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't Sequence[float] cover the ndarray cases that we actually want? i.e. once it is supported, something like np.ndarray[float, ndim=1] | None would be what we really want?

Copy link
Member Author

@simonjayhawkins simonjayhawkins Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of refine_percentiles is a numpy array and we pass this onto the other functions, so we need to add np.ndarray to the acceptable types.

doesn't Sequence[float] cover the ndarray cases that we actually want?

numpy/numpy#2776, issue has been opened a while, doesn't look like a fix in the pipeline

something like np.ndarray[float, ndim=1] | None would be what we really want?

probably not float, since int is also allowed. but if we narrow using float

from __future__ import annotations

import numpy as np
from typing import Sequence, Any

arr = np.array([0.25, 0.5])
reveal_type(arr)
arr2: np.ndarray[Any, np.dtype[np.float64]] = np.array([0.25, 0.5])
reveal_type(arr2)
arr3: np.ndarray[Any, np.dtype[np.float32]] = np.array([0.25, 0.5], dtype="float64")
reveal_type(arr3)
arr4: np.ndarray[Any, np.dtype[np.int_]] = np.array([0.25, 0.5], dtype="float64")
reveal_type(arr4)
arr5: np.ndarray[Any, np.dtype[np.int_]] = np.array([1, 2, 3])
reveal_type(arr5)


def test(a: Sequence[float]):
    pass


test(arr)
test(arr2)
test(arr3)
test(arr4)
test(arr5)


def test2(a: Sequence[float] | np.ndarray[Any, np.dtype[np.float_]]):
    reveal_type(a)


test2(arr)
test2(arr2)
test2(arr3)
test2(arr4)
test2(arr5)

/home/simon/t1.py:7: note: Revealed type is "numpy.ndarray[Any, Any]"
/home/simon/t1.py:9: note: Revealed type is "numpy.ndarray[Any, numpy.dtype[numpy.floating[numpy.typing._64Bit]]]"
/home/simon/t1.py:11: note: Revealed type is "numpy.ndarray[Any, numpy.dtype[numpy.floating[numpy.typing._32Bit]]]"
/home/simon/t1.py:13: note: Revealed type is "numpy.ndarray[Any, numpy.dtype[numpy.signedinteger[Any]]]"
/home/simon/t1.py:15: note: Revealed type is "numpy.ndarray[Any, numpy.dtype[numpy.signedinteger[Any]]]"
/home/simon/t1.py:22: error: Argument 1 to "test" has incompatible type "ndarray[Any, Any]"; expected "Sequence[float]"  [arg-type]
/home/simon/t1.py:23: error: Argument 1 to "test" has incompatible type "ndarray[Any, dtype[floating[_64Bit]]]"; expected "Sequence[float]"  [arg-type]
/home/simon/t1.py:24: error: Argument 1 to "test" has incompatible type "ndarray[Any, dtype[floating[_32Bit]]]"; expected "Sequence[float]"  [arg-type]
/home/simon/t1.py:25: error: Argument 1 to "test" has incompatible type "ndarray[Any, dtype[signedinteger[Any]]]"; expected "Sequence[float]"  [arg-type]
/home/simon/t1.py:26: error: Argument 1 to "test" has incompatible type "ndarray[Any, dtype[signedinteger[Any]]]"; expected "Sequence[float]"  [arg-type]
/home/simon/t1.py:30: note: Revealed type is "Union[typing.Sequence[builtins.float], numpy.ndarray[Any, numpy.dtype[numpy.floating[Any]]]]"
/home/simon/t1.py:36: error: Argument 1 to "test2" has incompatible type "ndarray[Any, dtype[signedinteger[Any]]]"; expected "Union[Sequence[float], ndarray[Any, dtype[floating[Any]]]]"  [arg-type]
/home/simon/t1.py:37: error: Argument 1 to "test2" has incompatible type "ndarray[Any, dtype[signedinteger[Any]]]"; expected "Union[Sequence[float], ndarray[Any, dtype[floating[Any]]]]"  [arg-type]
Found 7 errors in 1 file (checked 1 source file)

I've added the dtype bound to the return type of refine_percentiles since for return types we should be as precise as possible.

For the arguments of describe we want to be as permissible as possible, so maybe best not to narrow till the numpy inference works properly to avoid false positives for end users, describe is public.

'int' is duck type compatible with float, see https://mypy.readthedocs.io/en/stable/duck_type_compatibility.html#duck-type-compatibility, so having Sequence[float] is equivalent to Sequence[float | int]

on master

import numpy as np
import pandas as pd

print(pd.DataFrame(pd.Series([1, 2, 3])).describe([0, 1]))
print(pd.DataFrame(pd.Series([1, 2, 3])).describe([0.5]))
print(pd.DataFrame(pd.Series([1, 2, 3])).describe(np.array([0, 1])))
print(pd.DataFrame(pd.Series([1, 2, 3])).describe(np.array([0.5])))

gives

         0
count  3.0
mean   2.0
std    1.0
min    1.0
0%     1.0
50%    2.0
100%   3.0
max    3.0
         0
count  3.0
mean   2.0
std    1.0
min    1.0
50%    2.0
max    3.0
         0
count  3.0
mean   2.0
std    1.0
min    1.0
0%     1.0
50%    2.0
100%   3.0
max    3.0
         0
count  3.0
mean   2.0
std    1.0
min    1.0
50%    2.0
max    3.0

but on master, users don't see false positives since the percentiles parameter of DataFrame.describe is not yet typed

to type that, we need to pass the user passed argument onto the lower level functions, and these therefore need to be typed in a such a way to avoid false positives, so we can't wait for downstream fixes and imo best not to narrow types for numpy arrays till it's more mature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, thanks for walking me through that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, passing integers 0 and 1 is redundant anyway.

imo best not to narrow types for numpy arrays till it's more mature.

and for the internal functions, using the type parameters for np.ndarray is probably also fine. it's only the public api where we may want to be more cautious.

to be clear, while the constructors don't specialize and the dtype type parameter of np.ndarray is Any, then they will be type compatible with any specializations we do add. If we get these wrong, then there could be a batch of errors that need to be resolved when we get a future numpy release.

in numpy 1.21 (we use for type checking)...

def array(
    object: object,
    dtype: DTypeLike = ...,
    *,
    copy: bool = ...,
    order: _OrderKACF = ...,
    subok: bool = ...,
    ndmin: int = ...,
    like: ArrayLike = ...,
) -> ndarray: ...

so all arrays created with np.array will have an inferred type numpy.ndarray[Any, Any]

) -> FrameOrSeries:
"""Describe series or dataframe.

Expand Down Expand Up @@ -111,7 +112,7 @@ def __init__(self, obj: DataFrame | Series, datetime_is_numeric: bool):
self.datetime_is_numeric = datetime_is_numeric

@abstractmethod
def describe(self, percentiles: Sequence[float]) -> DataFrame | Series:
def describe(self, percentiles: Sequence[float] | np.ndarray) -> DataFrame | Series:
"""Do describe either series or dataframe.

Parameters
Expand All @@ -126,7 +127,7 @@ class SeriesDescriber(NDFrameDescriberAbstract):

obj: Series

def describe(self, percentiles: Sequence[float]) -> Series:
def describe(self, percentiles: Sequence[float] | np.ndarray) -> Series:
describe_func = select_describe_func(
self.obj,
self.datetime_is_numeric,
Expand Down Expand Up @@ -165,7 +166,7 @@ def __init__(

super().__init__(obj, datetime_is_numeric=datetime_is_numeric)

def describe(self, percentiles: Sequence[float]) -> DataFrame:
def describe(self, percentiles: Sequence[float] | np.ndarray) -> DataFrame:
data = self._select_data()

ldesc: list[Series] = []
Expand Down Expand Up @@ -387,18 +388,19 @@ def select_describe_func(
return describe_categorical_1d


def refine_percentiles(percentiles: Sequence[float] | None) -> Sequence[float]:
"""Ensure that percentiles are unique and sorted.
def refine_percentiles(
percentiles: Sequence[float] | np.ndarray | None,
) -> np.ndarray[Any, np.dtype[np.float64]]:
"""
Ensure that percentiles are unique and sorted.

Parameters
----------
percentiles : list-like of numbers, optional
The percentiles to include in the output.
"""
if percentiles is None:
# error: Incompatible return value type (got "ndarray", expected
# "Sequence[float]")
return np.array([0.25, 0.5, 0.75]) # type: ignore[return-value]
return np.array([0.25, 0.5, 0.75])

# explicit conversion of `percentiles` to list
percentiles = list(percentiles)
Expand All @@ -410,9 +412,7 @@ def refine_percentiles(percentiles: Sequence[float] | None) -> Sequence[float]:
if 0.5 not in percentiles:
percentiles.append(0.5)

# error: Incompatible types in assignment (expression has type "ndarray", variable
# has type "Optional[Sequence[float]]")
percentiles = np.asarray(percentiles) # type: ignore[assignment]
percentiles = np.asarray(percentiles)

# sort and check for duplicates
unique_pcts = np.unique(percentiles)
Expand Down