#25942 Added ArrayLike and Dtype to pandas._typing #25943
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25943 +/- ##
==========================================
- Coverage 91.81% 91.81% -0.01%
==========================================
Files 175 175
Lines 52580 52587 +7
==========================================
+ Hits 48278 48281 +3
- Misses 4302 4306 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25943 +/- ##
==========================================
+ Coverage 91.81% 91.83% +0.01%
==========================================
Files 175 175
Lines 52580 52524 -56
==========================================
- Hits 48278 48236 -42
+ Misses 4302 4288 -14
Continue to review full report at Codecov.
|
|
Ah yea makes sense
…Sent from my iPhone
On Apr 2, 2019, at 5:43 AM, Jeff Reback ***@***.***> wrote:
@jreback commented on this pull request.
In pandas/_typing.py:
>
+import numpy as np
+
+from pandas.core.dtypes.dtypes import ExtensionDtype
+from pandas.core.dtypes.generic import ABCExtensionArray
+
+ArrayLike = Union[ABCExtensionArray, np.ndarray]
+Dtype = Union[str, np.dtype, ExtensionDtype, Type]
@WillAyd Type is correct here or Type[int, float, object], not actul instances as indicated by @gwrome
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
pandas/core/arrays/sparse.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
this is too magical as this name gets shadowed immediately.
pandas/_typing.py
Outdated
| from pandas.core.dtypes.generic import ABCExtensionArray | ||
|
|
||
| ArrayLike = Union[ABCExtensionArray, np.ndarray] | ||
| SparseDtype = Union[str, np.dtype, ExtensionDtype, |
There was a problem hiding this comment.
I think maybe we should call this something else, as SparseDtype is an actual type that we have (see my comment below)
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Does that work for you, @jreback?
There was a problem hiding this comment.
yes @gwrome I think Dtype = Union[str, np.dtype, ExtensionDtype] looks reasonable assuming it works
There was a problem hiding this comment.
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
|
thanks @gwrome |
git diff upstream/master -u -- "*.py" | flake8 --diff