BUG: mixed-type mixed-timezone/awareness #55793
Conversation
There was a problem hiding this comment.
Really like the design you've set up with managing the parse state in its own class. Just a few comments but otherwise over to @MarcoGorelli
| if not tz_compare(tz_out, tz_out2): | ||
| # e.g. test_to_datetime_mixed_tzs_mixed_types | ||
| raise ValueError( | ||
| "Mixed timezones detected. pass utc=True in to_datetime " |
There was a problem hiding this comment.
Just to be clear this won't affect the object dtype case right? Maybe worth adding a test that is still possible if not already in the suite
There was a problem hiding this comment.
as in, pd.Index([datetime(2020, 1, 1, tzinfo=zoneinfo.ZoneInfo('US/Central')), datetime(2020, 1, 1, tzinfo=timezone.utc)])? I don't think it should affect it, but yeah, a little test with this wouldn't hurt
| @@ -14,6 +14,8 @@ cdef class DatetimeParseState: | |||
| cdef: | |||
| bint found_tz | |||
| bint found_naive | |||
| bint found_naive_str | |||
There was a problem hiding this comment.
Is it easy to add comments inline with these members or as part of the class? I can see the distinction between found_native and found_naive_str being unclear over time
There was a problem hiding this comment.
will update with comment(s). we'll be able to simplify this a ton in 3.0 once deprecation are enforced
| # both_strs-> paths that were previously already deprecated with warning | ||
| # issued in _array_to_datetime_object | ||
| both_strs = isinstance(aware_val, str) and isinstance(naive_val, str) | ||
| isinstance(aware_val, str) or isinstance(naive_val, str) |
There was a problem hiding this comment.
what's this statement doing?
There was a problem hiding this comment.
probably left over from a previous commit, will remove
| if not tz_compare(tz_out, tz_out2): | ||
| # e.g. test_to_datetime_mixed_tzs_mixed_types | ||
| raise ValueError( | ||
| "Mixed timezones detected. pass utc=True in to_datetime " |
There was a problem hiding this comment.
as in, pd.Index([datetime(2020, 1, 1, tzinfo=zoneinfo.ZoneInfo('US/Central')), datetime(2020, 1, 1, tzinfo=timezone.utc)])? I don't think it should affect it, but yeah, a little test with this wouldn't hurt
|
Thanks @jbrockmendel |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.Surfaced while implementing #55564.
Two implementation notes:
The implementation here is pretty ugly because we have to work around an existing deprecation. I chose to avoid changing the behavior in any of the cases that currently warn. Once that deprecation is enforced, we'll be able to simplify this quite a bit.
This takes a stand on API/PERF: when to check for mismatched tzs/awareness in array_to_datetime #55779 for checking once at the end instead of inside the loop.