Skip to content
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

#25942 Added ArrayLike and Dtype to pandas._typing #25943

Merged
merged 5 commits into from
Apr 10, 2019
Merged

#25942 Added ArrayLike and Dtype to pandas._typing #25943

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0761dab
#25942 Added ArrayLike and Dtype to pandas._typing
gwrome Mar 31, 2019
cc50a60
Revise and rename SparseDtype
gwrome Apr 3, 2019
3be8f63
Change import to SparseDtype
gwrome Apr 3, 2019
47b2cbb
Change type to SparseDtype
gwrome Apr 3, 2019
674c52f
Change SparseDtype back to Dtype
gwrome Apr 9, 2019
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
10 changes: 9 additions & 1 deletion pandas/_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
from pathlib import Path
from typing import IO, AnyStr, Union
from typing import IO, AnyStr, Type, Union

import numpy as np

from pandas.core.dtypes.dtypes import ExtensionDtype
from pandas.core.dtypes.generic import ABCExtensionArray

ArrayLike = Union[ABCExtensionArray, np.ndarray]
SparseDtype = Union[str, np.dtype, ExtensionDtype,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe we should call this something else, as SparseDtype is an actual type that we have (see my comment below)

cc @TomAugspurger

alternatively, maybe we just move this particular type definition into pandas/core/arrays.py and don't put it here (as I am not sure we are going to be using this particular type very often).

Copy link
Contributor Author

@gwrome gwrome Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think I misunderstood the comment above, then:

I think we need to use SparseDtype here explicity here

Did you mean inside the Union as part of the definition itself?

Copy link
Member

Choose a reason for hiding this comment

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

Might have missed the conversation but we just want to define Dtype here no?

Agreed SparseDtype in particular will have pretty limited usage, so not something I think we need to expose in this module

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i think we can make this only Dtype (so remove the Type references )

these are generic types so we want to use in many places (eg in pandas.core.dtypes.dtypes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just so I'm on the right page after all the discussion above, does rolling back to the first commit get us where we need to be?

That would return _typing.py to defining Dtype as is it currently defined in the existing type annotation, remove the specific Type references that were added after discussion above (Type[float], Type[int], Type[object]), and replace them with plain (and potentially over-broad) Type:

Dtype = Union[str, np.dtype, ExtensionDtype, Type]

Alternatively, do we want to go back to Dtype and drop all the Types? (This is how I interpret @jreback's "so remove the Type references" comment immediately above.) That would leave

Dtype = Union[str, np.dtype, ExtensionDtype]

I don't think this change would comply with the documented functionality of SparseDtype's init method that we pulled the definition from, but it does satisfy mypy.

Copy link
Member

Choose a reason for hiding this comment

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

The former should work here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that work for you, @jreback?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes @gwrome I think Dtype = Union[str, np.dtype, ExtensionDtype] looks reasonable assuming it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WillAyd for Dtype = Union[str, np.dtype, ExtensionDtype, Type]
@jreback for Dtype = Union[str, np.dtype, ExtensionDtype]

Both satisfy mypy. I propose we pick the more restrictive one: Dtype = Union[str, np.dtype, ExtensionDtype].

Copy link
Member

Choose a reason for hiding this comment

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

The only reason I was thinking we might need Type is because something like dtype=float is valid. OK to start smaller for now though - can add in later when it comes up

Type[float], Type[int], Type[object]]
FilePathOrBuffer = Union[str, Path, IO[AnyStr]]
3 changes: 2 additions & 1 deletion pandas/core/arrays/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
ABCExtensionArray, ABCIndexClass, ABCSeries)
from pandas.core.dtypes.missing import isna

from pandas._typing import ArrayLike
from pandas.core import ops

_not_implemented_message = "{} does not implement {}."
Expand Down Expand Up @@ -338,7 +339,7 @@ def astype(self, dtype, copy=True):
"""
return np.array(self, dtype=dtype, copy=copy)

def isna(self) -> Union[ABCExtensionArray, np.ndarray]:
def isna(self) -> ArrayLike:
"""
A 1-D array indicating if each value is missing.

Expand Down
5 changes: 3 additions & 2 deletions pandas/core/arrays/sparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import numbers
import operator
import re
from typing import Any, Callable, Type, Union
from typing import Any, Callable
import warnings

import numpy as np
Expand All @@ -30,6 +30,7 @@
ABCIndexClass, ABCSeries, ABCSparseArray, ABCSparseSeries)
from pandas.core.dtypes.missing import isna, na_value_for_dtype, notna

from pandas._typing import SparseDtype
Copy link
Contributor

Choose a reason for hiding this comment

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

this is too magical as this name gets shadowed immediately.

from pandas.core.accessor import PandasDelegate, delegate_names
import pandas.core.algorithms as algos
from pandas.core.arrays import ExtensionArray, ExtensionOpsMixin
Expand Down Expand Up @@ -79,7 +80,7 @@ class SparseDtype(ExtensionDtype):

def __init__(
self,
dtype: Union[str, np.dtype, ExtensionDtype, Type] = np.float64,
dtype: SparseDtype = np.float64,
fill_value: Any = None
) -> None:
from pandas.core.dtypes.missing import na_value_for_dtype
Expand Down