-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
TYP: misc fixes for numpy types #36098
Conversation
simonjayhawkins
commented
Sep 3, 2020
@@ -383,7 +383,9 @@ def extract_array(obj, extract_numpy: bool = False): | |||
if extract_numpy and isinstance(obj, ABCPandasArray): | |||
obj = obj.to_numpy() | |||
|
|||
return obj | |||
# error: Incompatible return value type (got "Index", expected "ExtensionArray") | |||
# error: Incompatible return value type (got "Series", expected "ExtensionArray") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid this by assigning to a new name before returning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look some more, but we are using ABC types here which always create issues with eliminating types so I just added the ignore for now so that the return types can be added.
@@ -62,7 +62,7 @@ | |||
# other | |||
|
|||
Dtype = Union[ | |||
"ExtensionDtype", str, np.dtype, Type[Union[str, float, int, complex, bool]] | |||
"ExtensionDtype", str, np.dtype, Type[Union[str, float, int, complex, bool, object]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object here is for when we use object
as a shorthand for np.dtype(object)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, but Dtype is also used to type the public facing api. ideally we don't want object here, but we don't want users to have false positives either. saying that we have not yet come to an agreement on how we will make the types public. see #28142
@@ -2316,7 +2316,7 @@ def _concat_same_type(self, to_concat): | |||
|
|||
return union_categoricals(to_concat) | |||
|
|||
def isin(self, values): | |||
def isin(self, values) -> np.ndarray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so here we know that this returns a bool dtyped ndarray, is it worth somehow annotating this as such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC there has been some discussion in numpy about making it viable to annotate something like np.ndarray[bool]
, but it is still hypothetical (cc @shoyer?)
@jbrockmendel good here? |
all good |