-
-
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
Conversation
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 this basically unified DatetimeBlock and DateimtTZBlock for _can_hold_element?
Yes. ATM DatetimeTZBlock._can_hold_element is ExtensionBlock._can_hold_element, which always returns True |
pandas/core/internals/blocks.py
Outdated
@@ -2647,7 +2663,7 @@ def _try_coerce_args(self, other): | |||
base-type other | |||
""" | |||
|
|||
if is_null_datetimelike(other): | |||
if is_null_datetimelike(other) and not isinstance(other, np.datetime64): |
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.
can you handle this case in is_null_datetimelike
lgtm. ping on green. |
The CI fail is weird. It’s getting a warning about elementwise comparison but it’s a scalar being compared. |
ping |
gentle pint; this is a semi-blocker |
pandas/core/internals/blocks.py
Outdated
return element.tzinfo is None | ||
elif is_integer(element): | ||
elif is_integer(element) and not isinstance(element, 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.
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.
(which ends up getting scattered).
Yah, hopefully-before-too-long-term i think the best solution is for all of these to dispatch to the TimedeltaArray/DatetimeArray implementations.
move this type of check to is_valid_nat_for_dtype
I considered that, but the trouble is that we really don't want iNaT
in there, see #16674. Maybe a stricter version of is_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.- numpy's deprecation warning about element-wise comparison is popping up when we compare
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.
Maybe a stricter version of is_integer?
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.
Looking at other uses of is_integer in this module, most of them would want to exclude timedelta64 objects
Does the whole world break if you just add the exclusion of timedelta64 to is_integer
(instead of making an is_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
pandas/core/internals/blocks.py
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
same as above comment
pandas/core/internals/blocks.py
Outdated
other = tslibs.iNaT | ||
elif ( |
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.
same
pandas/core/internals/blocks.py
Outdated
other = tslibs.iNaT | ||
elif ( |
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.
same
Thanks. +1 on the changes here, pending the resolution of |
ping |
thanks @jbrockmendel |
I've got a bunch of branches going that touch this, easier to just get it right once.