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.

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

#25790 Updating type hints to Python3 syntax in pandas/core/array #25829

Merged
merged 16 commits into from
Mar 30, 2019

Conversation

gwrome
Copy link
Contributor

@gwrome gwrome commented Mar 22, 2019

@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #25829 into master will decrease coverage by <.01%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25829      +/-   ##
==========================================
- Coverage   91.27%   91.27%   -0.01%     
==========================================
  Files         173      173              
  Lines       53002    53017      +15     
==========================================
+ Hits        48376    48389      +13     
- Misses       4626     4628       +2
Flag Coverage Δ
#multiple 89.83% <97.36%> (ø) ⬆️
#single 41.78% <97.36%> (+0.02%) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/base.py 98.33% <100%> (+0.05%) ⬆️
pandas/core/arrays/datetimelike.py 97.69% <100%> (+0.01%) ⬆️
pandas/core/arrays/integer.py 96.33% <100%> (+0.01%) ⬆️
pandas/core/arrays/period.py 98.53% <100%> (ø) ⬆️
pandas/core/arrays/sparse.py 92.19% <100%> (ø) ⬆️
pandas/core/arrays/datetimes.py 97.8% <100%> (ø) ⬆️
pandas/core/arrays/array_.py 97.87% <75%> (-2.13%) ⬇️
pandas/util/testing.py 89.3% <0%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e31b4f4...ea9c2b3. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #25829 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25829      +/-   ##
==========================================
+ Coverage   91.53%   91.56%   +0.02%     
==========================================
  Files         175      175              
  Lines       52808    52789      -19     
==========================================
- Hits        48338    48335       -3     
+ Misses       4470     4454      -16
Flag Coverage Δ
#multiple 90.12% <100%> (+0.02%) ⬆️
#single 41.83% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/datetimes.py 97.8% <ø> (ø) ⬆️
pandas/core/arrays/base.py 98.33% <100%> (+0.05%) ⬆️
pandas/core/arrays/datetimelike.py 97.69% <100%> (+0.01%) ⬆️
pandas/core/arrays/integer.py 96.33% <100%> (+0.01%) ⬆️
pandas/core/arrays/period.py 98.53% <100%> (ø) ⬆️
pandas/core/arrays/sparse.py 92.18% <100%> (ø) ⬆️
pandas/core/arrays/array_.py 100% <100%> (ø) ⬆️
pandas/compat/pickle_compat.py 69.51% <0%> (-6.1%) ⬇️
pandas/plotting/_compat.py 83.33% <0%> (-3.34%) ⬇️
pandas/compat/numpy/__init__.py 93.1% <0%> (-0.23%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 882961d...23aba8a. Read the comment docs.

copy=True, # type: bool
):
# type: (...) -> ExtensionArray
if TYPE_CHECKING:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? conditional imports for type checking are not supportable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand it, we need ExtensionArray to be defined to check this function's return type. Since ExtensionArray is defined elsewhere, we need to import it during type checking. I based this code on the approach described in PEP 484 — Runtime or type checking.

Is there a better, supportable way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

Yea this is the same conversation as #25802 (comment)

@jreback our support of 3.5 includes 3.5.0 and 3.5.1 right? This wasn't introduced until 3.5.2

Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead simply use the ABCExtensionArray classes which would work for this (and lots of other clases). we should maybe just do this generally. These are by-definition available with no dependencies for this exact purpose.

copy=True, # type: bool
):
# type: (...) -> ExtensionArray
if TYPE_CHECKING:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead simply use the ABCExtensionArray classes which would work for this (and lots of other clases). we should maybe just do this generally. These are by-definition available with no dependencies for this exact purpose.

@gfyoung gfyoung added the Typing type annotations, mypy/pyright type checking label Mar 25, 2019
@@ -343,8 +337,7 @@ def astype(self, dtype, copy=True):
"""
return np.array(self, dtype=dtype, copy=copy)

def isna(self):
# type: () -> Union[ExtensionArray, np.ndarray]
def isna(self) -> Union['ExtensionArray', np.ndarray]:
Copy link
Member

Choose a reason for hiding this comment

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

Might as well use the ABC here and throughout module

@@ -58,8 +58,7 @@ def _get_attributes_dict(self):
return {k: getattr(self, k, None) for k in self._attributes}

@property
def _scalar_type(self):
# type: () -> Union[type, Tuple[type]]
def _scalar_type(self) -> Union[type, Tuple[type]]:
Copy link
Member

Choose a reason for hiding this comment

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

Let's use Type consistently

scalars: Sequence[Optional[Period]],
dtype: PeriodDtype = None,
copy: bool = False,
) -> 'PeriodArray':
Copy link
Member

Choose a reason for hiding this comment

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

ABCPeriodArray

def _sparse_array_op(left, right, op, name):
# type: (SparseArray, SparseArray, Callable, str) -> Any
def _sparse_array_op(
left: 'SparseArray',
Copy link
Member

Choose a reason for hiding this comment

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

ABCSparseArray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're good to go @WillAyd @jreback

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

@gwrome one other thing that could be helpful - since some if not all of these modules are blacklisted from mypy could you manually run locally for the modules and compare this PR to head? Just want to make sure there are no new regressions

pandas/core/arrays/sparse.py Show resolved Hide resolved
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Couple minor things. IMO this is close - obviously some of these annotations are wrong and may need to be fixed but can be done in subsequent PRs

# TODO: ABCIndexClass is a valid type for other but had to be excluded
# due to length of Py2 compatability comment; add back in once migrated
# to Py3 syntax
other: Union[ExtensionArray, np.ndarray, ABCIndexClass],
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for removing the type for ndarray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During my testing at the time (before you added mypy.ini and the rest of the type checking framework to the project), mypy was throwing an error that suggested that ndarrays couldn't take a subscript, so I dropped it. I probably had something in my testing environment set up incorrectly (or maybe more strictly, not sure), because it doesn't throw that error with the current mypy.ini minus the blacklist. Will add it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, it wasn't mypy. code_checks doctests throws

...pandas/core/arrays/period.py", line 545, in PeriodArray
    other: Union[ExtensionArray, np.ndarray[int], ABCIndexClass],
TypeError: 'type' object is not subscriptable

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Kind of strange this throws an error now but not before. Something to revisit later

pandas/core/arrays/sparse.py Outdated Show resolved Hide resolved
@@ -343,8 +338,7 @@ def astype(self, dtype, copy=True):
"""
return np.array(self, dtype=dtype, copy=copy)

def isna(self):
# type: () -> Union[ExtensionArray, np.ndarray]
def isna(self) -> Union[ABCExtensionArray, np.ndarray]:
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is a the first stab at ArrayLike (e.g. to put into pandas.typing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a reasonable place to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

these are ok here for now, mainly adding things that we should add

Copy link
Member

Choose a reason for hiding this comment

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

Agreed this would be great @gwrome do you mind opening up an issue as a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I should be able to do that this weekend.

# type: (Union[str, np.dtype, 'ExtensionDtype', type], Any) -> None
def __init__(
self,
dtype: Union[str, np.dtype, ExtensionDtype, Type] = np.float64,
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be Dtype (in pandas.typing)

@jreback jreback added this to the 0.25.0 milestone Mar 28, 2019
@jreback
Copy link
Contributor

jreback commented Mar 30, 2019

lgtm. @WillAyd merge when ready

@WillAyd WillAyd merged commit f0ba498 into pandas-dev:master Mar 30, 2019
@WillAyd
Copy link
Member

WillAyd commented Mar 30, 2019

nice job @gwrome - thanks for helping to kick off these typing initiatives

@gwrome gwrome deleted the array-type-hints-python3 branch March 31, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants