Skip to content
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

BUG: Correct Timestamp localization with tz near DST (#11481) #15934

Merged
merged 2 commits into from
Apr 8, 2017

Conversation

mroeschke
Copy link
Member

Seemed that the Timestamp constructor with a string incorrectly localized a naive time with tz_convert_single instead of tz_localize_to_utc which is similar to how DatetimeIndex localizes.

Now the Timestamp will raise an error if an ambiguous combination of string and timezone is passed. Should this be the default behavior?

I had to slightly change 2 existing tests. test_timestamp_to_datetime_tzoffset used a now ambiguous timestamp and test_setitem_with_tz_dst had an incorrect expected timestamp.

@mroeschke mroeschke changed the title BUG: Timestamp doesn't respect tz DST (#11481) BUG: Correct Timestamp localization with tz near DST (#11481) Apr 7, 2017
@codecov
Copy link

codecov bot commented Apr 7, 2017

Codecov Report

Merging #15934 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15934      +/-   ##
==========================================
+ Coverage   90.99%   90.99%   +<.01%     
==========================================
  Files         145      145              
  Lines       49570    49570              
==========================================
+ Hits        45106    45107       +1     
+ Misses       4464     4463       -1
Flag Coverage Δ
#multiple 88.76% <ø> (ø) ⬆️
#single 40.56% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/tseries/index.py 95.43% <0%> (+0.09%) ⬆️

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 0aef74f...a7ee890. Read the comment docs.

@@ -1569,7 +1569,10 @@ cpdef convert_str_to_tsobject(object ts, object tz, object unit,
ts = obj.value
if tz is not None:
# shift for _localize_tso
ts = tz_convert_single(ts, tz, 'UTC')
#ts = tz_convert_single(ts, tz, 'UTC')
ts = tz_localize_to_utc(np.array([ts], dtype='i8'), tz,
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm there as to be an error in tz_convert_single. though it might simply be best to just replace that function to a call exactly like this, IOW (and see if anything else breaks)

cdef tz_convert_single(....)
   return tz_localize_to_utc(.....)[0]

@@ -159,6 +159,19 @@ def test_timestamp_constructed_by_date_and_tz_explicit(self):
self.assertEqual(result.hour, expected.hour)
self.assertEqual(result, expected)

def test_timestamp_constructor_near_dst_boundary(self):
# GH 11481 & 15777
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 give a 1 sentence on what the error was

@jreback
Copy link
Contributor

jreback commented Apr 7, 2017

can you scour the xref'ed issues for any more test cases and see if this closes any linked issues? (if you did already then ok).

@jreback jreback added Bug Timezones Timezone data dtype labels Apr 7, 2017
@mroeschke mroeschke force-pushed the fix_15777 branch 2 times, most recently from c03e06e to 848c383 Compare April 8, 2017 18:52
@mroeschke
Copy link
Member Author

Added some additional tests and described the error.

Addtionally, I tried dropping in tz_localize_to_utc to replace tz_convert_single and things were breaking. I think tz_convert_single is necessary in some places to convert from UTC to local timestamps (e.g. to show local time attributes instead of UTC time attributes)

@jreback
Copy link
Contributor

jreback commented Apr 8, 2017

thanks @mroeschke added some more tests. will merge on pass.

@jreback jreback merged commit f35209e into pandas-dev:master Apr 8, 2017
@mroeschke mroeschke deleted the fix_15777 branch December 20, 2017 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Timezones Timezone data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong hour of DST end for Europe TZ
2 participants