-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Properly handle UTC conversion in date2num. #6262
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
@@ -20,6 +22,8 @@ | |||
import matplotlib.pyplot as plt | |||
import matplotlib.dates as mdates | |||
|
|||
from numpy import array |
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.
Oops, this was an artifact of an earlier test strategy. I'll remove it now.
It may be worth adding a test that makes sure this works during a DST->STD transition, where there is a fold, but I suspect that in general that would mostly be a test of the time zone object's ability to handle ambiguous dates (see the extensive discussion at dateutil/dateutil#225 and the various linked discussions for more information than you could ever want about this), so I don't see any significant need for that. The best advice for people is that if they want to use |
I am glad someone who is not me understands dates 😄 . We do have some pandas-optional tests floating around already (I think mostly in test_axes) and install pandas on both travis and appveyor so if you want to use pandas you can. |
OK, I can add a pandas-specific test. Any idea why the Appveyor build is failing? I got that same failure locally on linux, but since I didn't touch anything related to that test, I figured it was just some weird version-specific problem. |
The appveyor is ses is likely due too #5950 |
0f66e41
to
9fdf115
Compare
9fdf115
to
66c0ed7
Compare
if td_remainder > 0: | ||
base += td_remainder / SEC_PER_DAY | ||
# Append the seconds as a fraction of a day | ||
base += _total_seconds(dt - rdt) / SEC_PER_DAY |
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.
Note: this _total_seconds
function was added for Python 2.6 compatibility, but based on the CI it seems that you've dropped Python 2.6 support. Have there been enough Python 2.6-breaking changes that it makes sense to drop this compatibility artifact?
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.
We are still testing 2.6 for the 1.5.x branch but other wise yes, remove
the 2.6 related stuff. We just will not backport this to 1.5.x (there
should not be a 1.5.2 release).
On Sat, Apr 2, 2016 at 5:31 PM Paul Ganssle notifications@github.com
wrote:
In lib/matplotlib/dates.py
#6262 (comment):
if td_remainder > 0:
base += td_remainder / SEC_PER_DAY
# Append the seconds as a fraction of a day
base += _total_seconds(dt - rdt) / SEC_PER_DAY
Note: this _total_seconds function was added for Python 2.6
compatibility, but based on the CI it seems that you've dropped Python 2.6
support. Have there been enough Python 2.6-breaking changes that it makes
sense to drop this compatibility artifact?—
You are receiving this because you commented.Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/6262/files/66c0ed7a019fb734de3615302923839bc692ba12#r58300209
bae6d54
to
3e6e8d1
Compare
Do y'all want me to rebase this so it passes the appveyor tests, or should I just leave it as is? |
# Convert to UTC | ||
tzi = getattr(dt, 'tzinfo', None) | ||
if tzi is not None: | ||
dt = dt.astimezone(UTC) |
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.
I don't think Python 2.7 datetime.date
has the astimezone
method.
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.
date
also doesn't have tzinfo
, so the astimezone
line will never fire.
Though, actually, datetime.datetime.timetz()
returns an object that has tzinfo
and not astimezone
. I'm not really sure how to deal with that, though. Are time
objects an acceptable input here? I think the offset returned will be None
if you try and get the utcoffset
of a non-fixed-offset bare time
object.
Pinging - any update on this PR? |
FIX: Properly handle UTC conversion in date2num
backported to v2.x as 64756ee |
Fixes #3896.
I believe this is a much better way to do it than the current method where we're trying to pull the
utcoffset
. The issue in #3896 actually has more to do with the waypytz
handles normalization and the way thatpandas
storesTimeStamp
objects. As you can see from the test I added, a normal list ofdatetime
objects withpytz
time zones won't trigger the bug, but rather than pulling inpandas
just for the test, I mocked out a datetime object that basically does the same thing pandas is doing.