-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: de-duplicate boxing in DTI.get_value #30819
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
else: | ||
key = Timestamp(key).tz_localize(self.tz) | ||
|
||
if isinstance(key, (datetime, np.datetime64)): | ||
return self.get_value_maybe_box(series, key) |
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.
the removed code here is duplicated in get_value_maybe_box
@jreback @jorisvandenbossche related to this PR I'm trying to de-duplicate the localization going on in _get_value_maybe_box. Back in #17920 it was decided to localize naive-looking strings when doing lookups on a tzaware DTI, but I'm surprised that we are doing the same for datetime objects (and AFAICT also np.datetime64, but we dont test that case) The relevant test is in tests.series.indexing.test_datetime, pulling out the pertinent part:
When I change the behavior of get_value to not cast tznaive datetime objects, this lookup raises (which I thought was the correct behavior, but apparently not?). Looking at the blame for this test, it has been around for a long time. Changing this would be non-trivial because it turns out that getitem[scalar] goes through DTI.get_value but slicing and setitem etc go through different paths. I guess there's not really a question here, just opening it up for comments. |
about your comment: #30819 (comment) yeah this is the implict conversion of strings only to a local timezone when they are naive (the strings) and the indexing is on timezone aware indexes. note that this does not apply to timestamps which must be explicityly timezoned. seems reasonable, but feel free to open an issue if more questions. ideally we should try to limit the indexing paths, but there is a lot going on so not sure how easy this is. |
thanks |
Also add a dedicated test or it to hit currently un-covered case of np.datetime64 key. That case works in master, but doesn't go through the expected code path