-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix _can_hold_element for datetimelike blocks #27347
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
Changes from 16 commits
0b5adeb
9a79b37
e1f8703
5c6ccd3
96e666f
3663e22
5a63cf4
36c3b58
e0f76a6
c26cd12
06b54ef
7a54804
6ae4ee1
07be36c
f4d381f
1b1d4d8
178dde6
060998e
a126c3b
aec6a6c
2f3a0ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,8 @@ | |
|
||
from pandas._libs import NaT, lib, tslib, tslibs | ||
import pandas._libs.internals as libinternals | ||
from pandas._libs.tslibs import Timedelta, conversion, is_null_datetimelike | ||
from pandas._libs.tslibs import Timedelta, conversion | ||
from pandas._libs.tslibs.timezones import tz_compare | ||
from pandas.util._validators import validate_bool_kwarg | ||
|
||
from pandas.core.dtypes.cast import ( | ||
|
@@ -60,7 +61,13 @@ | |
ABCPandasArray, | ||
ABCSeries, | ||
) | ||
from pandas.core.dtypes.missing import _isna_compat, array_equivalent, isna, notna | ||
from pandas.core.dtypes.missing import ( | ||
_isna_compat, | ||
array_equivalent, | ||
is_valid_nat_for_dtype, | ||
isna, | ||
notna, | ||
) | ||
|
||
import pandas.core.algorithms as algos | ||
from pandas.core.arrays import ( | ||
|
@@ -2248,14 +2255,17 @@ def _astype(self, dtype, **kwargs): | |
def _can_hold_element(self, element): | ||
tipo = maybe_infer_dtype_type(element) | ||
if tipo is not None: | ||
return tipo == _NS_DTYPE or tipo == np.int64 | ||
return is_dtype_equal(tipo, self.dtype) | ||
elif element is NaT: | ||
return True | ||
elif isinstance(element, datetime): | ||
if self.is_datetimetz: | ||
return tz_compare(element.tzinfo, self.dtype.tz) | ||
return element.tzinfo is None | ||
elif is_integer(element): | ||
elif is_integer(element) and not isinstance(element, np.timedelta64): | ||
return element == tslibs.iNaT | ||
|
||
# TODO: shouldnt we exclude timedelta64("NaT")? See GH#27297 | ||
return isna(element) | ||
return is_valid_nat_for_dtype(element, self.dtype) | ||
|
||
def _coerce_values(self, values): | ||
return values.view("i8") | ||
|
@@ -2275,8 +2285,14 @@ def _try_coerce_args(self, other): | |
------- | ||
base-type other | ||
""" | ||
if is_null_datetimelike(other): | ||
if is_valid_nat_for_dtype(other, self.dtype): | ||
other = tslibs.iNaT | ||
elif ( | ||
is_integer(other) | ||
and not isinstance(other, np.timedelta64) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above comment |
||
and other == tslibs.iNaT | ||
): | ||
pass | ||
elif isinstance(other, (datetime, np.datetime64, date)): | ||
other = self._box_func(other) | ||
if getattr(other, "tz") is not None: | ||
|
@@ -2359,6 +2375,8 @@ class DatetimeTZBlock(ExtensionBlock, DatetimeBlock): | |
is_datetimetz = True | ||
is_extension = True | ||
|
||
_can_hold_element = DatetimeBlock._can_hold_element | ||
|
||
@property | ||
def _holder(self): | ||
return DatetimeArray | ||
|
@@ -2465,8 +2483,14 @@ def _try_coerce_args(self, other): | |
# add the tz back | ||
other = self._holder(other, dtype=self.dtype) | ||
|
||
elif is_null_datetimelike(other): | ||
elif is_valid_nat_for_dtype(other, self.dtype): | ||
other = tslibs.iNaT | ||
elif ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
is_integer(other) | ||
and not isinstance(other, np.timedelta64) | ||
and other == tslibs.iNaT | ||
): | ||
pass | ||
elif isinstance(other, self._holder): | ||
if other.tz != self.values.tz: | ||
raise ValueError("incompatible or non tz-aware value") | ||
|
@@ -2606,12 +2630,16 @@ def _box_func(self): | |
def _can_hold_element(self, element): | ||
tipo = maybe_infer_dtype_type(element) | ||
if tipo is not None: | ||
# TODO: remove the np.int64 support once coerce_values and | ||
# _try_coerce_args both coerce to m8[ns] and not i8. | ||
return issubclass(tipo.type, (np.timedelta64, np.int64)) | ||
elif element is NaT: | ||
return True | ||
return is_integer(element) or isinstance( | ||
element, (timedelta, np.timedelta64, np.int64) | ||
) | ||
elif isinstance(element, (timedelta, np.timedelta64)): | ||
return True | ||
elif is_integer(element): | ||
return element == tslibs.iNaT | ||
return is_valid_nat_for_dtype(element, self.dtype) | ||
|
||
def fillna(self, value, **kwargs): | ||
|
||
|
@@ -2647,8 +2675,14 @@ def _try_coerce_args(self, other): | |
base-type other | ||
""" | ||
|
||
if is_null_datetimelike(other): | ||
if is_valid_nat_for_dtype(other, self.dtype): | ||
other = tslibs.iNaT | ||
elif ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
is_integer(other) | ||
and not isinstance(other, np.timedelta64) | ||
and other == tslibs.iNaT | ||
): | ||
pass | ||
elif isinstance(other, (timedelta, np.timedelta64)): | ||
other = Timedelta(other).value | ||
elif hasattr(other, "dtype") and is_timedelta64_dtype(other): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I would move this type of check to is_valid_nat_for_dtype to avoid placing logic in these routines (which ends up getting scattered).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, hopefully-before-too-long-term i think the best solution is for all of these to dispatch to the TimedeltaArray/DatetimeArray implementations.
I considered that, but the trouble is that we really don't want
iNaT
in there, see #16674. Maybe a stricter version ofis_integer
?This is definitely awkward; a couple of weird numpy behaviors are teaming up to make this necessary.
issubclass(np.timedelta64, np.integer)
returns True, which in turn makesis_integer(np.timedelta64(0))
return True.np.timedelta64(0) == iNaT
despite the fact that both are scalars.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at other uses of is_integer in this module, most of them would want to exclude timedelta64 objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's another place in this module where we already have the check
if is_integer(value) and not isinstance(value, np.timedelta64):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the whole world break if you just add the exclusion of timedelta64 to
is_integer
(instead of making anis_integer_strict
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will try this out in a dedicated branch