CLN: add typing for dtype arg in core/arrays (GH38808) #38886
Conversation
jreback
added
Dtype Conversions
| if is_interval_dtype(dtype): | ||
| dtype = cast(IntervalDtype, dtype) | ||
| if dtype.subtype is not None: | ||
| left = left.astype(dtype.subtype) | ||
| right = right.astype(dtype.subtype) | ||
| else: |
There was a problem hiding this comment.
why change the logic here? If the rest of the PR just adds type hints, maybe it would be safer to keep refactorings to a separate PR?
There was a problem hiding this comment.
I changed the code because otherwise the Mypy checks will not pass. But do note the logic itself did not change, it is just the order of operations
There was a problem hiding this comment.
this is fine (you just moved and changed the logical test), but puzzled why we don't need the clause on L229
There was a problem hiding this comment.
cc @jbrockmendel if any idea here
| if is_interval_dtype(dtype): | ||
| dtype = cast(IntervalDtype, dtype) | ||
| if dtype.subtype is not None: | ||
| left = left.astype(dtype.subtype) | ||
| right = right.astype(dtype.subtype) | ||
| else: |
There was a problem hiding this comment.
this is fine (you just moved and changed the logical test), but puzzled why we don't need the clause on L229
| if is_interval_dtype(dtype): | ||
| dtype = cast(IntervalDtype, dtype) | ||
| if dtype.subtype is not None: | ||
| left = left.astype(dtype.subtype) | ||
| right = right.astype(dtype.subtype) | ||
| else: |
There was a problem hiding this comment.
cc @jbrockmendel if any idea here
| right, | ||
| closed=None, | ||
| copy=False, | ||
| dtype: Optional[Dtype] = None, |
There was a problem hiding this comment.
by the time we get here we should have a DtypeObj shouldnt we?
There was a problem hiding this comment.
The _simple_new method is a.o. used in the __new__ method on L169 of this file. The __new__ method accepts the dtype values of type Dtype and passes it without modification to the dtype arg of _simple_new. Therefore I also gave it type Dtype here.
| @@ -223,12 +229,14 @@ def _simple_new( | |||
| if dtype is not None: | |||
| # GH 19262: dtype must be an IntervalDtype to override inferred | |||
| dtype = pandas_dtype(dtype) | |||
| if not is_interval_dtype(dtype): | |||
| if is_interval_dtype(dtype): | |||
There was a problem hiding this comment.
could just do isinstance(dtype, IntervalDtype) and avoid cast
There was a problem hiding this comment.
In #38680, we discussed the usage of isinstance vs is_* methods in combination with cast and the latter was preferred. Therefore I continued with it here.
pandas/core/arrays/interval.py
Outdated
| @@ -279,7 +287,7 @@ def _simple_new( | |||
| return result | |||
|
|
|||
| @classmethod | |||
| def _from_sequence(cls, scalars, *, dtype=None, copy=False): | |||
| def _from_sequence(cls, scalars, *, dtype: Optional[Dtype] = None, copy=False): | |||
| @@ -338,7 +346,9 @@ def _from_factorized(cls, values, original): | |||
| ), | |||
| } | |||
| ) | |||
| def from_breaks(cls, breaks, closed="right", copy=False, dtype=None): | |||
| def from_breaks( | |||
|
thanks @avinashpancham |
Follow on PR for #38808
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff