TYP: pandas/core/missing.py #38339

arw2019 · 2020-12-07T04:44:43Z

jorisvandenbossche · 2020-12-07T10:49:52Z

pandas/core/missing.py

-            )
+        if isna(order):
+            raise ValueError(f"order needs to be specified; got order: {order}")
+        assert isinstance(order, int)


We should not use asserts to please mypy if there is any risk for it to bubble up to the user (without it being a panda bug). I didn't look in enough detail to be sure here, but it looks like a user can pass eg a float here, and it should not raise an AssertionError.

xref #37350

I'm guessing this is done because isna won't narrow the type. If order is typed correct, order is None should be used instead of isna: order must either be a python integer or None. This eliminates the need for the assert.

I'm guessing this is done because isna won't narrow the type.

Yes that's the reason for this

If order is typed correct, order is None should be used instead of isna: order must either be a python integer or None. This eliminates the need for the assert.

Ok!

We should not use asserts to please mypy if there is any risk for it to bubble up to the user (without it being a panda bug). I didn't look in enough detail to be sure here, but it looks like a user can pass eg a float here, and it should not raise an AssertionError.

xref #37350

Fair point, noted

jorisvandenbossche · 2020-12-07T19:31:26Z

pandas/core/missing.py

-        method = alt_methods[method]
-        new_y = method(x, y, new_x, **kwargs)
+
+        assert isinstance(method, str)


same here: is there any guarantee we already checked this is a string by here?

i'm not sure (don't think so) but if it's not then we'll raise with/without the assert because in the line below method is a key to a dict where all keys are strings

pandas/pandas/core/missing.py

Lines 308 to 314 in 862cd05

# ignores some kwargs that could be passed along.

alt_methods = {

"barycentric": interpolate.barycentric_interpolate,

"krogh": interpolate.krogh_interpolate,

"from_derivatives": _from_derivatives,

"piecewise_polynomial": _from_derivatives,

}

I can add a more informative error message here, though

jorisvandenbossche · 2020-12-07T19:32:59Z

pandas/core/missing.py

+    xi: ArrayLike,
+    yi: ArrayLike,
+    x: Union[ArrayLike, Scalar],
+    axis: Optional[int] = 0,


How is axis optional? (it has a default of 0 already, so it's always specified?)

AFAIK Optional means it could be None.

That said, the docstring lists this arg as optional so I'll fix that while I'm here

Optional[T] in type-hinting is not the same as optional in docstrings. The former is an alias for Union[T, None] while the latter means doesn't need to be specified.

…ping-missing

jbrockmendel · 2020-12-08T17:23:11Z

pandas/core/missing.py

-):
+    x,
+    y,
+    new_x,


do we know anything about x, y, new_x?

yes - added

…ping-missing

jbrockmendel · 2020-12-09T16:29:33Z

pandas/core/missing.py

@@ -61,7 +63,7 @@ def mask_missing(arr: ArrayLike, values_to_mask) -> np.ndarray:
    return mask


-def clean_fill_method(method, allow_nearest: bool = False):
+def clean_fill_method(method: str, allow_nearest: bool = False) -> Optional[str]:


looks like method can be None?

good match (mypy missed this)

jbrockmendel · 2020-12-09T16:30:40Z

pandas/core/missing.py

+    start_nans = set() if start_nan_idx is None else set(range(start_nan_idx))
+
+    end_nan_idx = find_valid_index(yvalues, "last")
+    end_nans = set() if end_nan_idx is None else set(range(1 + end_nan_idx, len(valid)))


is this fixing a bug in the case where end_nan_idx is None?

I don't think so because we special case all nans/no nans at the top of the the method

jbrockmendel · 2020-12-09T16:31:22Z

pandas/core/missing.py

@@ -324,7 +338,7 @@ def _interpolate_scipy_wrapper(
    elif method == "cubicspline":
        alt_methods["cubicspline"] = _cubicspline_interpolate

-    interp1d_methods = [
+    interp1d_methods: List[str] = [


mypy cant figure this out on its own?

I think maybe I needed this at some point but yes mypy gets this without the annotation with the file as is

pandas/core/missing.py

jbrockmendel · 2020-12-09T16:33:39Z

pandas/core/missing.py

+    yi: ArrayLike,
+    x: Union[Scalar, ArrayLike],
+    der: Optional[int] = 0,
+    axis: Axis = 0,


i think just int; Axis includes "index" and "columns" which dont get resolved here

right, changed to int

jbrockmendel · 2020-12-09T16:34:14Z

pandas/core/missing.py

-def _akima_interpolate(xi, yi, x, der=0, axis=0):
+def _akima_interpolate(
+    xi: ArrayLike,
+    yi: ArrayLike,


the docstring says ArrayLike, but im guessing this wouldnt work on EAs

I guess not since this is a wrapper for scipy.interpolate.Akima1DInterpolator (and mypy works with both of these annotated asnp.ndarray)

jbrockmendel · 2020-12-09T16:34:48Z

pandas/core/missing.py

+    xi: ArrayLike,
+    yi: ArrayLike,
+    x: Union[Scalar, ArrayLike],
+    der: Optional[int] = 0,


is None really allowed?

no, don't think so. Changed to plain int

jbrockmendel · 2020-12-09T16:35:26Z

pandas/core/missing.py

@@ -572,12 +612,12 @@ def _interpolate_with_limit_area(


 def interpolate_2d(
-    values,
+    values: np.ndarray,
    method: str = "pad",
    axis: Axis = 0,


guessing just int

changed this to int

…ping-missing

jorisvandenbossche · 2020-12-10T09:09:52Z

pandas/core/missing.py

@@ -447,7 +480,14 @@ def _akima_interpolate(xi, yi, x, der=0, axis=0):
    return P(x, nu=der)


-def _cubicspline_interpolate(xi, yi, x, axis=0, bc_type="not-a-knot", extrapolate=None):
+def _cubicspline_interpolate(
+    xi: ArrayLike,


can be np.ndarray here as well, I think?

pandas/core/missing.py

simonjayhawkins · 2020-12-10T12:33:18Z

pandas/core/missing.py

@@ -23,7 +23,9 @@
    from pandas import Index


-def mask_missing(arr: ArrayLike, values_to_mask) -> np.ndarray:
+def mask_missing(
+    arr: ArrayLike, values_to_mask: Union[List, Tuple, Scalar]


values_to_mask is passed to infer_dtype_from_array

so the type of values_to_mask should be the same as the arr argument of infer_dtype_from_array

the arr argument of infer_dtype_from_array is not yet typed. (sometimes better to type the lower level functions first, but while np.ndarray resolves to Any, mypy won't report any inconsistencies)

looking quickly through the body of infer_dtype_from_array, Series and EA are handled, so passing an array-like to values_to_mask should be fine.

To handle Series, could either add to Union, or could maybe (if no perf implications) also update infer_dtype_from_array to handle Index as well, and use AnyArrayLike instead.

Adding the Index check to infer_dtype_from_array

diff --git a/pandas/core/dtypes/cast.py b/pandas/core/dtypes/cast.py index 165e63e23d..676ab572a0 100644 --- a/pandas/core/dtypes/cast.py +++ b/pandas/core/dtypes/cast.py @@ -83,6 +83,7 @@ from pandas.core.dtypes.generic import ( ABCDatetimeArray, ABCDatetimeIndex, ABCExtensionArray, + ABCIndexClass, ABCPeriodArray, ABCPeriodIndex, ABCSeries, @@ -877,7 +878,7 @@ def infer_dtype_from_array( if pandas_dtype and is_extension_array_dtype(arr): return arr.dtype, arr - elif isinstance(arr, ABCSeries): + elif isinstance(arr, ABCSeries) or isinstance(arr, ABCIndexClass): return arr.dtype, np.asarray(arr) # don't force numpy coerce with nan's

seems to degrade some of the benchmarks that hit this

asv continuous -f 1.1 upstream/master HEAD -b ^algorithms -b ^replace before after ratio [d0db0098] [9b685722] <master~1> <master> + 3.41±0.04ms 5.40±0.7ms 1.59 algorithms.Factorize.time_factorize(False, False, 'boolean') + 4.50±0.06ms 6.98±0.9ms 1.55 algorithms.Factorize.time_factorize(False, True, 'boolean') + 3.27±0.01μs 3.89±0.5μs 1.19 algorithms.Duplicated.time_duplicated(True, 'first', 'float') SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. PERFORMANCE DECREASED.

so I guess a Union with Series is the way to go unless there's a more efficient patch for infer_dtype_from_array

Thanks for looking into this. Looking again, I see that Index is already handled and falls through the the final return after np.asarray(arr). so could probably use Union[AnyArrayLike, Sequence[Any], Scalar] after all.

Right - done

…ping-missing

jreback · 2020-12-14T14:31:58Z

pandas/core/dtypes/cast.py

@@ -815,8 +822,8 @@ def dict_compat(d: Dict[Scalar, Scalar]) -> Dict[Scalar, Scalar]:


 def infer_dtype_from_array(
-    arr, pandas_dtype: bool = False
-) -> Tuple[DtypeObj, ArrayLike]:
+    arr: "Union[ArrayLike, Series, PandasScalar]", pandas_dtype: bool = False


I think for these just use AnyArrayLike

jreback · 2020-12-14T14:32:24Z

pandas/core/missing.py

@@ -23,7 +33,9 @@
    from pandas import Index


-def mask_missing(arr: ArrayLike, values_to_mask) -> np.ndarray:
+def mask_missing(
+    arr: ArrayLike, values_to_mask: Union[AnyArrayLike, Scalar, Sequence[Any]]


AnyArrayLike

jreback · 2020-12-14T14:33:51Z

pandas/core/missing.py

        How many derivatives to extract; None for all potentially nonzero
        derivatives (that is a number equal to the number of points), or a
        list of derivatives to extract. This number includes the function
        value as 0th derivative.
-     extrapolate : bool, optional
+     extrapolate : bool, default False


the alignmnet here is off

jreback · 2020-12-14T14:34:15Z

pandas/core/missing.py

+def _akima_interpolate(
+    xi: np.ndarray,
+    yi: np.ndarray,
+    x: Union[Scalar, ArrayLike],


i think these are just np.ndarray right?

and similar in almost all places that you are using Union[Scalar, ArrayLike] unless you can reveal_type that this is not the case

You're right, they're always ndarray except in _interpolate_scipy_wrapper. It's because in _interpolate_scipy_wrapper we cast any scalar input to ndarray and these functions always get called from there:

pandas/pandas/core/missing.py

Line 306 in e91cbd7

new_x = np.asarray(new_x)

Fixed these, and docstrings too

…ping-missing

jreback · 2021-02-11T00:38:29Z

@arw2019 this is somewhat old, so maybe simpler to start a new PR, or merge master and update; as always on typing if can do the unambiguous ones first

arw2019 · 2021-02-11T01:27:20Z

@arw2019 this is somewhat old, so maybe simpler to start a new PR, or merge master and update; as always on typing if can do the unambiguous ones first

I'm thinking I'll factor out the easy ones into a separate PR so this is easier to look at and maybe get that merged first

(FWIW this got a lot of review so i'm confident the hard ones here are mostly right at this point)

jorisvandenbossche · 2021-02-24T09:01:03Z

pandas/core/missing.py

@@ -39,7 +44,9 @@
    from pandas import Index


-def mask_missing(arr: ArrayLike, values_to_mask) -> np.ndarray:
+def mask_missing(
+    arr: AnyArrayLike, values_to_mask: Union[AnyArrayLike, Scalar, Sequence[Any]]


I think the ArrayLike for arr was correct here?
(or if not, the docstring needs to be updated as well)

simonjayhawkins · 2021-03-14T10:34:28Z

@arw2019 can you merge master and fixup mypy failures

simonjayhawkins · 2021-03-15T13:42:18Z

@arw2019 this is somewhat old, so maybe simpler to start a new PR, or merge master and update; as always on typing if can do the unambiguous ones first

@arw2019 closing this. can you break this up and open several new smaller PRs

simonjayhawkins · 2021-03-15T13:47:34Z

@arw2019 if you add an ignore, cast or assert, or change/delete an existing annotation or change any actual code, put those changes a minimally scoped PR.

add type hints

830fa00

arw2019 force-pushed the typing-missing branch from f3a0fea to 830fa00 Compare December 7, 2020 05:32

jorisvandenbossche reviewed Dec 7, 2020

View reviewed changes

arw2019 added 4 commits December 7, 2020 11:13

review: remove assert

605dc3c

merge master

e77c940

typo

f2d5ec4

add isna check

e83904f

jorisvandenbossche reviewed Dec 7, 2020

View reviewed changes

arw2019 added 5 commits December 7, 2020 16:46

better error msg when interp method not string

71caeeb

improve docstring

8fbbd47

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

4474ada

…ping-missing

remove Optional

575c227

use Axis TypeVar

b19896b

jbrockmendel reviewed Dec 8, 2020

View reviewed changes

arw2019 added 2 commits December 8, 2020 13:14

more hints

5036ee1

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

c0c4338

…ping-missing

jbrockmendel reviewed Dec 9, 2020

View reviewed changes

pandas/core/missing.py Show resolved Hide resolved

jbrockmendel reviewed Dec 9, 2020

View reviewed changes

arw2019 added 3 commits December 9, 2020 12:52

review comments

2a31823

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

4fb893b

…ping-missing

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

95a734b

…ping-missing

jorisvandenbossche reviewed Dec 10, 2020

View reviewed changes

simonjayhawkins reviewed Dec 10, 2020

View reviewed changes

arw2019 added 5 commits December 10, 2020 13:22

review comment

4aeec70

review comment: values_to_mask

d67977d

review comments: mask_missing/infer_dtype_from_array

24f418a

typo

aeb0b82

typo

25d0051

arw2019 added the Typing type annotations, mypy/pyright type checking label Dec 11, 2020

arw2019 added 3 commits December 11, 2020 12:26

review comment

bbd25ed

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

785d27c

…ping-missing

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

c2d6467

…ping-missing

jreback requested changes Dec 14, 2020

View reviewed changes

arw2019 added 4 commits December 14, 2020 17:14

review comment

b505de5

docstring fix

e39c152

review comments

cb82c9a

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

65effed

…ping-missing

arw2019 added the Needs Review label Dec 22, 2020

Merge branch 'master' of https://github.com/pandas-dev/pandas into ty…

2fa64bd

…ping-missing

arw2019 added 3 commits February 21, 2021 13:13

merge master

a54a02f

TYP: infer_dtype_from_array

315822c

minimize diff

df4b70a

jorisvandenbossche reviewed Feb 24, 2021

View reviewed changes

simonjayhawkins removed the Needs Review label Mar 14, 2021

simonjayhawkins closed this Mar 15, 2021

	# ignores some kwargs that could be passed along.
	alt_methods = {
	"barycentric": interpolate.barycentric_interpolate,
	"krogh": interpolate.krogh_interpolate,
	"from_derivatives": _from_derivatives,
	"piecewise_polynomial": _from_derivatives,
	}

TYP: pandas/core/missing.py #38339

TYP: pandas/core/missing.py #38339

Conversation

arw2019 commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 11, 2021

arw2019 commented Feb 11, 2021

Choose a reason for hiding this comment

simonjayhawkins commented Mar 14, 2021

simonjayhawkins commented Mar 15, 2021

simonjayhawkins commented Mar 15, 2021