Assert at least one tz arg is always UTC #18228

jbrockmendel · 2017-11-11T16:37:06Z

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

gfyoung · 2017-11-11T18:55:11Z

pandas/_libs/tslibs/conversion.pyx

@@ -422,6 +422,9 @@ cpdef int64_t tz_convert_single(int64_t val, object tz1, object tz2):
        pandas_datetimestruct dts
        datetime dt

+    assert (is_utc(tz1) or tz1 == 'UTC') or (is_utc(tz2) or tz2 == 'UTC')
+    # See GH#17734


Let's the bug the comment above the line.

Perhaps a one sentence explanation for this assertion would be good too.

If a user were to stumble upon this assert, wouldn't we want some kind of error message?

gfyoung · 2017-11-11T18:57:04Z

pandas/_libs/tslibs/conversion.pyx

@@ -487,6 +490,9 @@ def tz_convert(ndarray[int64_t] vals, object tz1, object tz2):
        pandas_datetimestruct dts
        datetime dt

+    assert (is_utc(tz1) or tz1 == 'UTC') or (is_utc(tz2) or tz2 == 'UTC')
+    # See GH#17734


Same as above

jreback · 2017-11-11T21:10:31Z

pandas/_libs/tslibs/conversion.pyx

@@ -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
    if is_tzlocal(tz2):
-        dt64_to_dtstruct(val, &dts)
+        dt64_to_dtstruct(utc_date, &dts)


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

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.

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.

codecov · 2017-11-12T00:00:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@40fd6b4). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #18228   +/-   ##
=========================================
  Coverage          ?   91.39%           
=========================================
  Files             ?      163           
  Lines             ?    50091           
  Branches          ?        0           
=========================================
  Hits              ?    45779           
  Misses            ?     4312           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.2% <ø> (?)`
#single	`40.36% <ø> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40fd6b4...754dd7b. Read the comment docs.

jreback · 2017-11-12T15:37:37Z

pandas/_libs/tslibs/conversion.pyx

@@ -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
    if is_tzlocal(tz2):
-        dt64_to_dtstruct(val, &dts)
+        dt64_to_dtstruct(utc_date, &dts)


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.

jreback · 2017-11-12T15:38:25Z

pandas/_libs/tslibs/conversion.pyx

@@ -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
    if is_tzlocal(tz2):


jreback · 2017-11-12T15:38:33Z

pandas/_libs/tslibs/conversion.pyx

@@ -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


return val (see my comment below)

jbrockmendel · 2017-11-12T17:49:41Z

remove the first clause else (which just sets utc_date = val, then this is no longer necessary.

I think there may be some confusion about the underlying issue. In the case where utc_date = val, you're right that the change in line 448/451 is irrelevant. The problem is that in the case where utc_date != val, the change here may matter.

The new assertion here is a band-aid that ensures we never get to cases where the change here matters. Ideally we shouldn't need that assertion.

The status-quo use of val in line 448 is an outlier that does not fit with either a) the rest of this function or b) the implementation in tz_convert. The latter is the bug in question. Changing val --> utc_date in line 451 is very much a good thing.

…libs-fix_tzconvert

jreback · 2017-11-12T18:04:59Z

ok ping on green.

note you may wish to switch / use rebasing rather than merging as its much cleaner.

jbrockmendel · 2017-11-12T18:23:38Z

ok ping on green.

The PR in its current state is a win because it avoids incorrect behavior, but if we can confirm that the usage in tz_convert (which is here the new usage in tz_convert_single) is correct, then we can get rid of the assertion.

On the flip side, if we cant confirm that the usage in tz_convert is correct, then we have an altogether different problem.

note you may wish to switch / use rebasing rather than merging as its much cleaner.

This distinction has come up over in statsmodels. I've just been running "git pull upstream master" whenever a rebase is needed. I'll try to figure out the grown-up way of doing things.

jreback · 2017-11-12T18:31:19Z

This distinction has come up over in statsmodels. I've just been running "git pull upstream master" whenever a rebase is needed. I'll try to figure out the grown-up way of doing things.

when we merge it doesn't matter as things get squashed to a single commit.

its cleaner for reviewing; every little commit doesn't matter. Logical commits matter.

jreback · 2017-11-12T19:34:07Z

thanks @jbrockmendel

closes pandas-dev#17734

jbrockmendel added 2 commits November 11, 2017 08:36

Assert at least one tz arg is always UTC

81f8a33

closes pandas-dev#17734

update asserts

65f001d

gfyoung added Bug Timezones Timezone data dtype labels Nov 11, 2017

gfyoung reviewed Nov 11, 2017

View reviewed changes

jbrockmendel added 2 commits November 11, 2017 11:19

edit comment per reviewer suggestion

563510c

only assert for tz_convert_single

359aeff

jreback reviewed Nov 11, 2017

View reviewed changes

typo fix

17dd7ab

jreback requested changes Nov 12, 2017

View reviewed changes

jbrockmendel added 2 commits November 12, 2017 09:51

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

c1aa846

…libs-fix_tzconvert

edit per reviewer request

754dd7b

jreback added this to the 0.22.0 milestone Nov 12, 2017

jreback approved these changes Nov 12, 2017

View reviewed changes

jreback merged commit 5e553ce into pandas-dev:master Nov 12, 2017

jreback mentioned this pull request Nov 22, 2017

Tslibs offsets validation #18210

Closed

4 tasks

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

Assert at least one tz arg is always UTC (pandas-dev#18228)

6f988d7

closes pandas-dev#17734

jbrockmendel deleted the tslibs-fix_tzconvert branch December 8, 2017 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert at least one tz arg is always UTC #18228

Assert at least one tz arg is always UTC #18228

jbrockmendel commented Nov 11, 2017

gfyoung Nov 11, 2017 •

edited

Loading

gfyoung Nov 11, 2017

jreback Nov 11, 2017

jbrockmendel Nov 11, 2017 •

edited

Loading

jreback Nov 12, 2017

codecov bot commented Nov 12, 2017 •

edited

Loading

jreback Nov 12, 2017

jreback Nov 12, 2017

jreback Nov 12, 2017

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jreback commented Nov 12, 2017

		@@ -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

Assert at least one tz arg is always UTC #18228

Assert at least one tz arg is always UTC #18228

Conversation

jbrockmendel commented Nov 11, 2017

gfyoung Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

gfyoung Nov 11, 2017

Choose a reason for hiding this comment

jreback Nov 11, 2017

Choose a reason for hiding this comment

jbrockmendel Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

jreback Nov 12, 2017

Choose a reason for hiding this comment

codecov bot commented Nov 12, 2017 • edited Loading

Codecov Report

jreback Nov 12, 2017

Choose a reason for hiding this comment

jreback Nov 12, 2017

Choose a reason for hiding this comment

jreback Nov 12, 2017

Choose a reason for hiding this comment

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jreback commented Nov 12, 2017

gfyoung Nov 11, 2017 •

edited

Loading

jbrockmendel Nov 11, 2017 •

edited

Loading

codecov bot commented Nov 12, 2017 •

edited

Loading