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

TYP: indexes #37224

Merged
merged 14 commits into from
Oct 29, 2020
Merged

TYP: indexes #37224

merged 14 commits into from
Oct 29, 2020

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

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.

A few comments

pandas/core/indexes/datetimelike.py Outdated Show resolved Hide resolved
pandas/core/indexes/period.py Outdated Show resolved Hide resolved
@@ -463,6 +464,7 @@ def get_result(self):
if self.bm_axis == 0:
name = com.consensus_name_attr(self.objs)
cons = self.objs[0]._constructor
assert issubclass(cons, Series) # for mypy
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the assert I think we can just annotate the _constructor property (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

it already is annotated

Copy link
Member

Choose a reason for hiding this comment

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

I recall a discussion about this. There is no requirement that _constructor has to be type(self). This makes typing intractable in many places so we assume that _constructor is type(self).

Copy link
Member

Choose a reason for hiding this comment

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

Could this raise AssertionError for users with subclassed Series and DataFrames?

probably safer to use casts here. (anywhere where _constructor is involved)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Using cons = cast(Type[Series], cons) here leads to errors:

pandas/core/reshape/concat.py:467: error: Incompatible types in assignment (expression has type "Type[Series]", variable has type "Type[FrameOrSeries]")  [assignment]
pandas/core/reshape/concat.py:472: error: Unexpected keyword argument "index" for "NDFrame"  [call-arg]
pandas/core/generic.py:213: note: "NDFrame" defined here
pandas/core/reshape/concat.py:472: error: Unexpected keyword argument "name" for "NDFrame"  [call-arg]
pandas/core/generic.py:213: note: "NDFrame" defined here
pandas/core/reshape/concat.py:472: error: Unexpected keyword argument "dtype" for "NDFrame"  [call-arg]

Copy link
Member

@simonjayhawkins simonjayhawkins Oct 27, 2020

Choose a reason for hiding this comment

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

hmm, early in the history of adding type hints it was decided that Aliases should be TypeVars. This can cause many issues.

here it maybe that objs on L298 should use FrameOrSeriesUnion.

Anyway, working with the TypeVar for now. there are two issues.

  1. mypy cannot perform type narrowing with if self._is_series
  2. FrameOrSeries is bounded by NDFrame, so mypy cannot know that if not a Series it is a DataFrame (in the else).

Since you've added from pandas import DataFrame, Series could do...

diff --git a/pandas/core/reshape/concat.py b/pandas/core/reshape/concat.py
index e54ad177b3..9dba9be0ff 100644
--- a/pandas/core/reshape/concat.py
+++ b/pandas/core/reshape/concat.py
@@ -3,7 +3,7 @@ Concat routines.
 """
 
 from collections import abc
-from typing import TYPE_CHECKING, Iterable, List, Mapping, Union, overload
+from typing import TYPE_CHECKING, Iterable, List, Mapping, Type, Union, overload

 import numpy as np

@@ -457,14 +457,15 @@ class _Concatenator:
     def get_result(self):
         from pandas import DataFrame, Series

+        cons: Type[FrameOrSeriesUnion]
+
         # series only
-        if self._is_series:
+        if isinstance(self.objs[0], Series):

             # stack blocks
             if self.bm_axis == 0:
                 name = com.consensus_name_attr(self.objs)
                 cons = self.objs[0]._constructor
-                assert issubclass(cons, Series)  # for mypy

                 arrs = [ser._values for ser in self.objs]

@@ -478,7 +479,6 @@ class _Concatenator:

                 # GH28330 Preserves subclassed objects through concat
                 cons = self.objs[0]._constructor_expanddim
-                assert issubclass(cons, DataFrame)  # for mypy

                 index, columns = self.new_axes
                 df = cons(data, index=index)
@@ -487,6 +487,7 @@ class _Concatenator:

         # combine block managers
         else:
+            assert isinstance(self.objs[0], DataFrame)  # for mypy
             mgrs_indexers = []
             for obj in self.objs:
                 indexers = {}

Copy link
Member

Choose a reason for hiding this comment

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

Since you've added from pandas import DataFrame, Series could do...

casts would be my preference but also needs a few hoops to avoid Incompatible types in assignment with TypeVars.

diff --git a/pandas/core/reshape/concat.py b/pandas/core/reshape/concat.py
index e54ad177b3..e894c2a373 100644
--- a/pandas/core/reshape/concat.py
+++ b/pandas/core/reshape/concat.py
@@ -3,7 +3,7 @@ Concat routines.
 """
 
 from collections import abc
-from typing import TYPE_CHECKING, Iterable, List, Mapping, Union, overload
+from typing import TYPE_CHECKING, Iterable, List, Mapping, Type, Union, cast, overload
 
 import numpy as np
 
@@ -30,7 +30,7 @@ import pandas.core.indexes.base as ibase
 from pandas.core.internals import concatenate_block_managers
 
 if TYPE_CHECKING:
-    from pandas import DataFrame
+    from pandas import Series, DataFrame
     from pandas.core.generic import NDFrame
 
 # ---------------------------------------------------------------------
@@ -455,16 +455,17 @@ class _Concatenator:
         self.new_axes = self._get_new_axes()
 
     def get_result(self):
-        from pandas import DataFrame, Series
+        cons: Type[FrameOrSeriesUnion]
+        sample: FrameOrSeriesUnion
 
         # series only
         if self._is_series:
+            sample = cast("Series", self.objs[0])
 
             # stack blocks
             if self.bm_axis == 0:
                 name = com.consensus_name_attr(self.objs)
-                cons = self.objs[0]._constructor
-                assert issubclass(cons, Series)  # for mypy
+                cons = sample._constructor
 
                 arrs = [ser._values for ser in self.objs]
 
@@ -477,8 +478,7 @@ class _Concatenator:
                 data = dict(zip(range(len(self.objs)), self.objs))
 
                 # GH28330 Preserves subclassed objects through concat
-                cons = self.objs[0]._constructor_expanddim
-                assert issubclass(cons, DataFrame)  # for mypy
+                cons = sample._constructor_expanddim
 
                 index, columns = self.new_axes
                 df = cons(data, index=index)
@@ -487,6 +487,7 @@ class _Concatenator:
 
         # combine block managers
         else:
+            sample = cast("DataFrame", self.objs[0])
             mgrs_indexers = []
             for obj in self.objs:
                 indexers = {}
@@ -509,7 +510,7 @@ class _Concatenator:
             if not self.copy:
                 new_data._consolidate_inplace()
 
-            cons = self.objs[0]._constructor
+            cons = sample._constructor
             return cons(new_data).__finalize__(self, method="concat")
 
     def _get_result_dim(self) -> int:

@@ -476,6 +478,7 @@ def get_result(self):

# GH28330 Preserves subclassed objects through concat
cons = self.objs[0]._constructor_expanddim
assert issubclass(cons, DataFrame) # for mypy
Copy link
Member

Choose a reason for hiding this comment

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

Same comment

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Oct 19, 2020
@jreback jreback added this to the 1.2 milestone Oct 19, 2020
@@ -161,6 +161,24 @@ def to_timestamp(self, freq=None, how="start") -> DatetimeIndex:
arr = self._data.to_timestamp(freq, how)
return DatetimeIndex._simple_new(arr, name=self.name)

# error: Decorated property not supported [misc]
Copy link
Member

Choose a reason for hiding this comment

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

also need to remove from inherit_names?

and maybe update comment on L151

Copy link
Member Author

Choose a reason for hiding this comment

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

updated comment; removing from inherit_names is a PITA since it isnt there directly. harmless to have it there.

@simonjayhawkins
Copy link
Member

@jbrockmendel can you respond to #37224 (comment)

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @jbrockmendel lgtm

@WillAyd WillAyd merged commit 5532ae8 into pandas-dev:master Oct 29, 2020
@WillAyd
Copy link
Member

WillAyd commented Oct 29, 2020

Thanks!

@jbrockmendel jbrockmendel deleted the typ-gb branch October 29, 2020 15:55
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
ukarroum pushed a commit to ukarroum/pandas that referenced this pull request Nov 2, 2020
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.

None yet

4 participants