Skip to content

Assert at least one tz arg is always UTC #18228

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.

Sign up for GitHub

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 7 commits into from
Nov 12, 2017
Merged

Assert at least one tz arg is always UTC #18228

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
81f8a33
Assert at least one tz arg is always UTC
jbrockmendel Nov 11, 2017
65f001d
update asserts
jbrockmendel Nov 11, 2017
563510c
edit comment per reviewer suggestion
jbrockmendel Nov 11, 2017
359aeff
only assert for tz_convert_single
jbrockmendel Nov 11, 2017
17dd7ab
typo fix
jbrockmendel Nov 11, 2017
c1aa846
Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…
jbrockmendel Nov 12, 2017
754dd7b
edit per reviewer request
jbrockmendel Nov 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pandas/_libs/tslibs/conversion.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,9 @@ cpdef int64_t tz_convert_single(int64_t val, object tz1, object tz2):
pandas_datetimestruct dts
datetime dt

# See GH#17734 We should always be converting either from UTC or to UTC
assert (is_utc(tz1) or tz1 == 'UTC') or (is_utc(tz2) or tz2 == 'UTC')

if val == NPY_NAT:
return val

Expand All @@ -445,7 +448,7 @@ cpdef int64_t tz_convert_single(int64_t val, object tz1, object tz2):
if get_timezone(tz2) == 'UTC':
return utc_date
Copy link
Contributor

Choose a reason for hiding this comment

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

return val (see my comment below)

if is_tzlocal(tz2):
Copy link
Contributor

Choose a reason for hiding this comment

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

elif

dt64_to_dtstruct(val, &dts)
dt64_to_dtstruct(utc_date, &dts)
Copy link
Contributor

Choose a reason for hiding this comment

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

huh? this seems like it would break something, if it doesn't that's a problem

Copy link
Member Author

@jbrockmendel jbrockmendel Nov 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The bug here is an inconsistency between tz_convert and tz_convert_single. The edit here makes tz_convert_single behave the same as tz_convert.

The reason why this hasn't broken anything is because in all existing usages+tests of tz_convert_single, one of tz1 or tz2 is UTC. As long as this condition holds, the two versions of the code are equivalent. This PR adds an assertion that this condition holds.

Of course, assuming the tz_convert implementation is correct, once the implementation here is changed, the assertion is no longer necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think one of my comments is gone. remove the first clause else (which just sets utc_date = val, then this is no longer necessary.

dt = datetime(dts.year, dts.month, dts.day, dts.hour,
dts.min, dts.sec, dts.us, tz2)
delta = int(get_utcoffset(tz2, dt).total_seconds()) * 1000000000
Expand Down