Skip to content

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

Merged
merged 21 commits into from
Jul 15, 2019

Conversation

jbrockmendel
Copy link
Member

I've got a bunch of branches going that touch this, easier to just get it right once.

Copy link
Contributor

@jreback jreback left a 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?

@jreback jreback added Clean Internals Related to non-user accessible pandas implementation labels Jul 11, 2019
@jbrockmendel
Copy link
Member Author

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

@@ -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):
Copy link
Contributor

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

@jreback jreback added this to the 0.25.0 milestone Jul 12, 2019
@jreback
Copy link
Contributor

jreback commented Jul 12, 2019

lgtm. ping on green.

@jbrockmendel
Copy link
Member Author

The CI fail is weird. It’s getting a warning about elementwise comparison but it’s a scalar being compared.

@jbrockmendel
Copy link
Member Author

ping

@jbrockmendel
Copy link
Member Author

gentle pint; this is a semi-blocker

return element.tzinfo is None
elif is_integer(element):
elif is_integer(element) and not isinstance(element, np.timedelta64):
Copy link
Contributor

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).

Copy link
Member Author

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.

  1. issubclass(np.timedelta64, np.integer) returns True, which in turn makes is_integer(np.timedelta64(0)) return True.
  2. 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.

Copy link
Member Author

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

Copy link
Member Author

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):

Copy link
Contributor

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)?

Copy link
Member Author

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

other = tslibs.iNaT
elif (
is_integer(other)
and not isinstance(other, np.timedelta64)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above comment

other = tslibs.iNaT
elif (
Copy link
Contributor

Choose a reason for hiding this comment

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

same

other = tslibs.iNaT
elif (
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@TomAugspurger
Copy link
Contributor

will try this out in a dedicated branch

Thanks. +1 on the changes here, pending the resolution of is_integer / is_integer_strict (can we think of a different suffix than _strict? That doesn't really convey that (just) time delta are excluded).

@jbrockmendel
Copy link
Member Author

Does the whole world break if you just add the exclusion of timedelta64 to is_integer

Nope, just opened #27401

I think the thing to do here is to revert the last commit to get rid of is_integer_strict and move forward with the more verbose variant, then trim that if/when #27401 goes in.

@jbrockmendel
Copy link
Member Author

ping

@jreback jreback merged commit 7b61952 into pandas-dev:master Jul 15, 2019
@jreback
Copy link
Contributor

jreback commented Jul 15, 2019

thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the can_hold branch July 15, 2019 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants