TYP: ExtensionArray delete() and searchsorted() #41513

Dr-Irv · 2021-05-16T14:30:46Z

Additional typing for ExtensionArray delete() and searchsorted()

pandas/core/arrays/base.py

simonjayhawkins

Thanks @Dr-Irv . now that the EA methods are being annotated individually, it may make sense to add the annotations to all EA classes for that method to ensure consistency. (e.g. SparseArray looks iffy def searchsorted(self, v, side="left", sorter=None):

pandas/core/arrays/base.py

simonjayhawkins · 2021-05-17T08:54:47Z

pandas/core/arrays/base.py

@@ -1308,7 +1313,7 @@ def __hash__(self) -> int:
    # ------------------------------------------------------------------------
    # Non-Optimized Default Methods

-    def delete(self: ExtensionArrayT, loc) -> ExtensionArrayT:
+    def delete(self: ExtensionArrayT, loc: PositionalIndexer) -> ExtensionArrayT:


This is in the 'public' base EA class. it was added in #39405. @jbrockmendel is this method public? Is it part of the EA interface? does it need a docstring?

good catch ill make a note to add a docstring

Dr-Irv · 2021-05-17T15:12:22Z

Thanks @Dr-Irv . now that the EA methods are being annotated individually, it may make sense to add the annotations to all EA classes for that method to ensure consistency. (e.g. SparseArray looks iffy def searchsorted(self, v, side="left", sorter=None):

Next commit does this. Had to create some types to support the bridge from our searchsorted to numpy searchsorted. Hope this works for you!

simonjayhawkins · 2021-05-22T15:30:43Z

pandas/core/base.py

+        value: NumpyValueArrayLike,
+        side: Literal["left", "right"] = "left",
+        sorter: NumpySorter = None,
+    ) -> np.ndarray:


this can sometimes return an int?

searchsorted returns an np.ndarray according to the numpy docs and the signature in numpy 1.20.

looks like that doc/signature is inaccurate then

>>> arr = np.arange(5) >>> type(arr.searchsorted(3)) <class 'numpy.int64'>

So this seems to be an issue with numpy. This is using numpy 1.20.2:

import numpy as np a = np.array([1, 3, 5]) # reveal_type(a) s = a.searchsorted(4) # reveal_type(s) print(s, type(s), isinstance(s, np.ndarray))

Result of running this is:

2 <class 'numpy.int64'> False

If I uncomment the reveal_type lines, the types of a and s are both np.ndarray.

So that gives us this question:

Should the pandas searchsorted API match what numpy documents (only array elements allowed), or what numpy implements?

I've submitted a numpy issue here: numpy/numpy#19160

pandas/_typing.py

pandas/core/algorithms.py

pandas/core/indexes/extension.py

Dr-Irv · 2021-05-30T17:50:19Z

@simonjayhawkins @jbrockmendel pushed changes 5 days ago, so looking forward to a new review.

pandas/core/algorithms.py

jbrockmendel · 2021-05-30T20:52:15Z

pandas/core/arrays/_mixins.py

-        return self._ndarray.searchsorted(value, side=side, sorter=sorter)
+    def searchsorted(
+        self,
+        value: ArrayLike | object,


object -> Scalar?

Would violate the Liskov substitution principle. Our searchsorted needs to have the EA's reject an incompatible type

I guess that would be bc you've typed EA.searchsorted with object, so why not change it there?

Because in EA, we have to allow any type to be inside the EA, including non-EA types.

ArrayLike is a subtype of object so not needed? Why does this not use the NumpyValueArrayLike alias?

pandas/core/arrays/_mixins.py

jbrockmendel · 2021-05-30T20:55:11Z

pandas/core/arrays/period.py

-        value = self._validate_searchsorted_value(value).view("M8[ns]")
+    def searchsorted(
+        self,
+        value: ArrayLike | object,


i think de-facto the scalar needs to be either Period or Period-castable

Yes, but the ExtensionArray type has to allow any object, so the subtype does as well, and then it should raise if someone passes an incompatible object type.

has to allow any object

aren't we supposed to use Any for that?

so the subtype does as well, and then it should raise if someone passes an incompatible object type

so if a subclass is more restrictive in what it accepts (without raising), we can't have that be reflected in the annotation?

has to allow any object

aren't we supposed to use Any for that?

If we use Any, then that tells the type checker to ignore type checking on the result. If we use object, then it will continue type checking.

so the subtype does as well, and then it should raise if someone passes an incompatible object type

so if a subclass is more restrictive in what it accepts (without raising), we can't have that be reflected in the annotation?

Yes and no. The full signature has to accept the same type as the parent, and can widen that argument type, but not narrow it. But, I think we can use an overload signature to indicate that if a specific type is passed, it creates a result of that type. So the master signature could accept object, and then you can have an overload to correspond to the narrower type.

jbrockmendel · 2021-05-30T20:57:44Z

all the Literal["left", "right"] changes look good. the NumpySorter is fine AFAIK. the value arg in searchsorted is pretty nasty though; can we live without it for the time being?

Dr-Irv · 2021-05-31T16:12:24Z

all the Literal["left", "right"] changes look good. the NumpySorter is fine AFAIK. the value arg in searchsorted is pretty nasty though; can we live without it for the time being?

I think we should go ahead as I proposed it. The issue with value is that ExtensionArray has to support any type for a singleton, so it takes object. Therefore the subtypes have to take any type, and ideally they should raise if the type is incompatible.

github-actions · 2021-07-04T00:02:31Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

Dr-Irv · 2021-07-09T13:02:51Z

@jbrockmendel Awaiting your response for 6 weeks to my last response here: #41513 (comment)

jbrockmendel · 2021-07-09T14:03:45Z

The issue with value is that ExtensionArray has to support any type for a singleton, so it takes object.

You explained here why you are using object instead of Any. With object, the checker treats it as if that argument were not annotated (because everything matches), while with Any the checker skips the method completely. Do I have that right?

Therefore the subtypes have to take any type, and ideally they should raise if the type is incompatible.

The full signature has to accept the same type as the parent, and can widen that argument type, but not narrow it

IIUC "has to" here is in order to satisfy Liskov. Is that right? Liskov is nice, but in this context it is conflicting with accuracy. e.g. in the PeriodArray case it is inaccurately suggesting that it will accept a timedelta. If we have to choose between Liskov and accuracy, my inclination is to choose accuracy. And if I'm right above that putting "object" is equivalent to leaving it unannotated, then this is introducing inaccuracy with no upside AFAICT.

That said, this is not a hill I want to die on. I'll defer to @simonjayhawkins

Dr-Irv · 2021-07-26T12:59:51Z

pinging @simonjayhawkins

jreback · 2021-07-28T01:40:59Z

@Dr-Irv this looks pretty good to me, can you rebase and hopefully @simonjayhawkins can have a look

Dr-Irv · 2021-08-03T18:14:04Z

weekly ping @simonjayhawkins

simonjayhawkins

can you fix conflicts and a mypy error pandas/core/indexes/datetimes.py:772: error: Argument "side" to "get_slice_bound" of "Index" has incompatible type "str"; expected "Union[Literal['left'], Literal['right']]" [arg-type]

Thanks @Dr-Irv for being patient. In future maybe worth starting at the lower level functions and work up to the public api and maybe tackle a subset of the method arguments. Gradual typing facilitates this.

simonjayhawkins · 2021-08-11T14:50:16Z

pandas/core/arrays/_mixins.py

-        return self._ndarray.searchsorted(value, side=side, sorter=sorter)
+    def searchsorted(
+        self,
+        value: ArrayLike | object,


ArrayLike is a subtype of object so not needed? Why does this not use the NumpyValueArrayLike alias?

simonjayhawkins · 2021-08-11T14:51:39Z

pandas/core/arrays/_mixins.py

+        return self._ndarray.searchsorted(npvalue, side=side, sorter=sorter)
+
+    def _validate_searchsorted_value(
+        self, value: ArrayLike | object


again, ArrayLike is a subtype of object?

Changed this to now correspond better to the numpy signature. No longer refer to object

simonjayhawkins · 2021-08-11T14:57:08Z

pandas/core/arrays/_mixins.py

+        else:
+            # While we also can support DatetimeLikeScalar, numpy.searchsorted
+            # does not accept that as an argument
+            return cast(PythonScalar, value)


There are code changes here from master, this is a typing PR. Is this is fixing a bug? It looks like this prevents passing an EA to numpy? Does numpy currently raise a TypeError or does it use the __array__ protocol? Does this need a test?

mypy complains when you pass an ExtensionArray to searchsorted. E.g.:

import numpy as np import pandas as pd from pandas.core.arrays.base import ExtensionArray from typing import cast a = pd.array([1,3,4]) b = cast(ExtensionArray, a) na = np.array([1,2,3,4,5]) r = na.searchsorted(b) print(r)

produces from mypy:

irv.py:14: error: No overload variant of "searchsorted" of "ndarray" matches argument type "ExtensionArray" [call-overload] irv.py:14: note: Possible overload variants: irv.py:14: note: def searchsorted(self, v: Union[int, float, complex, str, bytes, generic], side: Union[Literal['left'], Literal['right']] = ..., sorter: Union[_SupportsArray[dtype[Union[bool_, integer[Any]]]], Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]], Sequence[Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]]]]], int, Sequence[int], Sequence[Sequence[int]], Sequence[Sequence[Sequence[int]]], Sequence[Sequence[Sequence[Sequence[int]]]], None] = ...) -> signedinteger[Any] irv.py:14: note: def searchsorted(self, v: Union[Sequence[Sequence[Sequence[Sequence[Sequence[Any]]]]], Union[Union[_SupportsArray[dtype[Any]], Sequence[_SupportsArray[dtype[Any]]], Sequence[Sequence[_SupportsArray[dtype[Any]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]]]], Union[bool, int, float, complex, str, bytes, Sequence[Union[bool, int, float, complex, str, bytes]], Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]], Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]], Sequence[Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]]]]]], side: Union[Literal['left'], Literal['right']] = ..., sorter: Union[_SupportsArray[dtype[Union[bool_, integer[Any]]]], Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]], Sequence[Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Union[bool_, integer[Any]]]]]]]], int, Sequence[int], Sequence[Sequence[int]], Sequence[Sequence[Sequence[int]]], Sequence[Sequence[Sequence[Sequence[int]]]], None] = ...) -> ndarray[Any, dtype[signedinteger[Any]]]

So while mypy accepts the ExtensionArray argument, the typing that is present within mypy does not.

simonjayhawkins · 2021-08-11T14:58:33Z

pandas/core/arrays/_mixins.py

+            # While we also can support DatetimeLikeScalar, numpy.searchsorted
+            # does not accept that as an argument


numpy will raise TypeError for this case?

This is removed in next commit.

simonjayhawkins · 2021-08-11T15:05:36Z

pandas/core/arrays/_mixins.py

+        if isinstance(value, ExtensionArray):
+            return value.to_numpy()
+        elif isinstance(value, np.ndarray):
+            return value


this condition and the following condition both return value, the cast is a noop at runtime. can this be simplified?

Yes, fixed in next commit

simonjayhawkins · 2021-08-11T15:50:41Z

pandas/core/arrays/_mixins.py

+        value: ArrayLike | object,
+        side: Literal["left", "right"] = "left",
+        sorter: NumpySorter = None,
+    ) -> np.ndarray:


Yes, np.intp in next commit

simonjayhawkins · 2021-08-11T15:51:08Z

pandas/core/arrays/base.py

+        value: ArrayLike | object,
+        side: Literal["left", "right"] = "left",
+        sorter: NumpySorter = None,
+    ) -> np.ndarray:


Yes, np.intp in next commit.

simonjayhawkins · 2021-08-11T15:52:34Z

pandas/core/arrays/period.py

+        npvalue = cast(
+            np.ndarray, self._validate_searchsorted_value(value).view("M8[ns]")
+        )


can we avoid the cast with the typing of _validate_searchsorted_value

removed in next commit.

simonjayhawkins · 2021-08-11T15:54:14Z

pandas/core/base.py

+        value: NumpyValueArrayLike,
+        side: Literal["left", "right"] = "left",
+        sorter: NumpySorter = None,
+    ) -> npt.NDArray[np.intp]:


for consistency can you update the return types of the other methods to also use the npt.NDArray[np.intp] syntax.

done in next commit

simonjayhawkins · 2021-08-11T15:55:24Z

pandas/core/computation/pytables.py

+                # "Union[Any, ndarray[Any, Any]]", variable has type "int")
+                result = metadata.searchsorted(
+                    v, side="left"
+                )  # type: ignore[assignment]


can avoid the ignore using a variable annotation for result?

Done in next commit

Dr-Irv · 2021-08-22T22:34:51Z

@simonjayhawkins Have just made a bunch of changes to use more numpy types in arguments and return types for searchsorted.

One odd thing was that I put the overloads for searchsorted that numpy uses only in pandas/core/indexes/extension.py. Alternatively, I could put this everywhere, BUT as you'll see in the comments in that file, it requires a type: ignore statement, which is also present in the mypy definition of searchsorted() in numpy/__init__.pyi.

I could put the same overloads throughout the code, but it would require the type: ignore messages in every place. Let me know which you prefer.

Dr-Irv · 2021-09-06T15:46:21Z

biweekly ping @simonjayhawkins

jreback · 2021-09-06T15:48:16Z

@Dr-Irv ok if you can merge master and make sure still passing (i think it was last) time, will merge these.

Dr-Irv · 2021-09-06T17:04:24Z

@jreback all green except for a Windows timeout on one of the tests

jreback · 2021-09-06T17:10:31Z

thanks @Dr-Irv followup to consolidate any fixtures created here in other PRs

Dr-Irv added 2 commits May 16, 2021 10:20

add typing for ExtensionArray.delete

1bf4b5d

typing searchsorted

16bcc1a

Dr-Irv mentioned this pull request May 16, 2021

TYP: Update type hints for ExtensionArray and ExtensionDtype #39501

Closed

jbrockmendel reviewed May 17, 2021

View reviewed changes

pandas/core/arrays/base.py Outdated Show resolved Hide resolved

simonjayhawkins reviewed May 17, 2021

View reviewed changes

simonjayhawkins added the Typing type annotations, mypy/pyright type checking label May 17, 2021

searchsorted typing updates throughout

c0d426f

searchsorted typing updates throughout

cce79a9

simonjayhawkins reviewed May 22, 2021

View reviewed changes

simonjayhawkins added the ExtensionArray Extending pandas with custom dtypes or arrays. label May 22, 2021

jbrockmendel reviewed May 25, 2021

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed May 25, 2021

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed May 25, 2021

View reviewed changes

pandas/core/algorithms.py Show resolved Hide resolved

jbrockmendel reviewed May 25, 2021

View reviewed changes

pandas/core/indexes/extension.py Outdated Show resolved Hide resolved

Dr-Irv added 2 commits May 25, 2021 21:47

Merge remote-tracking branch 'upstream/master' into extdelete

0a9cd7a

remove NumpyArrayLike and fix arguments to index.searchsorted

f37d215

jbrockmendel reviewed May 30, 2021

View reviewed changes

pandas/core/algorithms.py Show resolved Hide resolved

jbrockmendel reviewed May 30, 2021

View reviewed changes

pandas/core/arrays/_mixins.py Outdated Show resolved Hide resolved

jbrockmendel reviewed May 30, 2021

View reviewed changes

Merge branch 'master' into extdelete

22b147a

Dr-Irv added 2 commits May 31, 2021 12:37

validate_search_sorted fixup

065454f

fix to NDArrayBackedExtensionArray._validate_searchsorted_value

cde198e

github-actions bot added the Stale label Jul 4, 2021

merge with master - just doc changes

b986eba

merge with master - just doc changes

2d85ddb

Dr-Irv removed the Stale label Jul 4, 2021

resolve conflicts

493ab41

merge with master

f8f8b93

jreback added this to the 1.4 milestone Jul 28, 2021

Dr-Irv added 3 commits July 28, 2021 08:15

Merge remote-tracking branch 'upstream/master' into extdelete

704abef

Merge remote-tracking branch 'upstream/master' into extdelete

f8d4267

merge with master 0803

9c391a0

simonjayhawkins reviewed Aug 11, 2021

View reviewed changes

Dr-Irv added 2 commits August 22, 2021 18:28

Use numpy types in arguments and return types for searchsorted

4e60e04

Small fix to validate_searchsorted_value

6a888ef

add periods to docstrings

2370384

Merge remote-tracking branch 'upstream/master' into extdelete

42802ec

jreback merged commit 11a8c17 into pandas-dev:master Sep 6, 2021

Dr-Irv deleted the extdelete branch September 6, 2021 17:35

feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021

TYP: ExtensionArray delete() and searchsorted() (pandas-dev#41513)

4867cf9

		# While we also can support DatetimeLikeScalar, numpy.searchsorted
		# does not accept that as an argument

TYP: ExtensionArray delete() and searchsorted() #41513

TYP: ExtensionArray delete() and searchsorted() #41513

Conversation

Dr-Irv commented May 16, 2021

simonjayhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr-Irv commented May 17, 2021

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

Dr-Irv commented May 30, 2021

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

jbrockmendel commented May 30, 2021

Dr-Irv commented May 31, 2021

github-actions bot commented Jul 4, 2021

Dr-Irv commented Jul 9, 2021

jbrockmendel commented Jul 9, 2021

Dr-Irv commented Jul 26, 2021

jreback commented Jul 28, 2021

Dr-Irv commented Aug 3, 2021

simonjayhawkins left a 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

Dr-Irv commented Aug 22, 2021

Dr-Irv commented Sep 6, 2021

jreback commented Sep 6, 2021

Dr-Irv commented Sep 6, 2021

jreback commented Sep 6, 2021