FIX: properly set tz for YearLocator #12678

jklymak · 2018-10-30T20:28:22Z

PR Summary

dates.YearLocator was not always being called with the timezone info. Now it is...

import matplotlib.pyplot as plt
import pytz
from datetime import datetime, timedelta, timezone

x = [datetime(2010, 1, 1).astimezone(pytz.timezone('America/New_York')) + timedelta(i) for i in range(2000)]
plt.plot(x, [0] * len(x));
plt.show()

Now has the first tick at 2010 instead of 2009....

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

QuLogic · 2018-10-30T20:33:37Z

Add/modify a test?

jklymak · 2018-10-30T20:35:48Z

Yeah, good point - we should have caught this in testing before.

jklymak · 2018-10-30T21:46:54Z

test added...

jklymak · 2018-10-30T23:18:38Z

I have a bad feeling that this solution will fail on 3.5 for some datetime objects. As of 3.6 you can do datetime.astimezone, but before 3.6 you cannot if there is no timezone info. So I'm re-milestoning this fix for 3.1. There may be a way to work around this for 3.0.x, but I'm not clear what it is.

jklymak · 2018-10-31T03:05:40Z

OK, this will fail for py 3.5 if the user supplies a naive datetime (no timezone info) and they are using the YearLocator, but will work otherwise. Added a slightly less mysterious error message for that case.

jklymak · 2018-10-31T05:06:09Z

test_constrainedlayout.py::test_colorbar_location seems to be a bit of a flaky test. its a bit complicated, involves a solver, and sometimes seems to fail.

tacaswell · 2018-11-04T04:37:21Z

lib/matplotlib/dates.py

+        try:
+            ticks = [vmin.astimezone(self.tz)]
+        except ValueError:
+            raise ValueError('naive datetime objects cannot be used '


This may be a good place to use the raise ... from e functionality of python3 (https://www.python.org/dev/peps/pep-3134/)

try: ticks = [vmin.astimezone(self.tz)] except ValueError as E: raise ValueError() from E

OH, Cool, didn't know about that. I'll do soon.

tacaswell · 2018-11-04T04:37:56Z

lib/matplotlib/dates.py

                         }
+        # Note that tzinfo does not work with pytz timezones, and


This is moot if we drop py3.5?

ah, this is targetted at 3.0.x so have to support 3.5 still.

Yep... Could milestone 3.1 if you prefer.

tacaswell · 2018-11-04T04:41:06Z

attn @pganssle for advise on how to deal with timezones cleanly....

tacaswell · 2018-11-04T04:42:15Z

Requiring timezone aware datetimes to use the YearLocator seems like a pretty big API change? Am I misunderstanding this?

jklymak · 2018-11-04T04:45:03Z

No, this just fixes the YearLocator - it wasn't getting the timezone information passed to it properly, so 1 Jan 00:00 sometimes looked like 31 Dec 23:00, and the year was formatted as the previous year. This shouldn't affect the other Locators or Formatters in the autodateLocator (I'd hope we'd fail a few tests if it did!).

pganssle · 2018-11-04T16:10:39Z

lib/matplotlib/tests/test_dates.py

    for t_delta, expected in results:
        d2 = d1 + t_delta
        locator = _create_auto_date_locator(d1, d2)
        assert list(map(str, mdates.num2date(locator()))) == expected


+@pytest.mark.pytz
+@pytest.mark.skipif(not __has_pytz(), reason="Requires pytz")


Why are you using pytz for this? Tests with pytz are just to ensure matplotlib is compatible with pytz, but this is a general functionality test. It's better to use dateutil.tz, which is not conditional on pytz installation.

The original bug report was pytz, and pytz doesn't work if the code is not as written in the PR. i.e. I needed to test pytz to get the PR to work, so it makes sense to test it here...

Is it not possible to reproduce the bug with dateutil.tz? Just because the reporter used pytz doesn't mean you can't create an MWE without that dependency.

Just checked, the original bug is also present when you use dateutil.tz, with the original MWE now (more accurately in this case):

import matplotlib.pyplot as plt from dateutil import tz from datetime import datetime, timedelta x = [datetime(2010, 1, 1, tzinfo=tz.gettz('America/New_York')) + timedelta(i) for i in range(2000)] plt.plot(x, [0] * len(x));

jklymak · 2018-11-04T17:38:45Z

lib/matplotlib/dates.py

                         }
+        # Note that tzinfo does not work with pytz timezones, and


@pganssle : the way that tz info was put in before was to do vmin.replace(year=ymin, **self.replaced), but pytz doesn't work properly (at all) with replace. So we need a test with pytz to test that this code works.

To be honest, we roundtrip most dates (datetime-> ordinal -> datetime) so the fact that pytz doesn't do the right thing w/ tzinfo isn't that big a deal practically, but it would be nice if it worked.

I guess we could add another test that doesn't use pytz, but I think pytz should be tested on this code path.

replace(tzinfo=tz) and astimezone(tz) do two very different things.

If you think of time zones as units for datetimes, replace(tzinfo=tz) is like getting a value that says "1 ft", scratching off the "ft" and adding "m" to get "1m". astimezone(tz) is like getting something that says 1ft and converting it to meters to get 0.3m.

Generally speaking, replace is used if you know that the local portion of the datetime is correct and want to say what time zone it's in. astimezone is used if you know that the absolute time represented by the datetime is correct and you want to convert it to a different local time.

In Python >=3.6, astimezone can be used on naive datetimes, but what that does is that it interprets the naive datetime as local time, and converts that to the time zone of interest. What that means is that if you are in Tokyo and give this a naive datetime like datetime(2018, 1, 1) and have YearLocator(tz=tz.gettz("America/New_York"), that's going to get converted to 2017-12-31 10:00:00-05:00, which is probably not what you wanted.

pytz doesn't work properly (at all) with replace. So we need a test with pytz to test that this code works.

Also, I'll note that I'm well aware of the limitations of pytz.

I think probably it's best to use two tests, one with pytz (skipped if pytz is not present) and one with dateutil. If you intend for the interface to look the same, you can likely parametrize the test function with a conditional skip on pytz.

pganssle · 2018-11-04T18:04:10Z

I haven't totally grokked how your code works, but it's worth noting that I solved a similar problem in the RRULE locator some time back, so RRuleLocator should be able to handle time zones correctly. The way it works in rrulewrapper is that if an RRULE has an attached time zone, that time zone is stripped off, the RRULE generates dates, then the time zone is re-attached before the datetime object is emitted.

I think you probably want to do something similar for YearlyLocator.

timhoffm

I would create a cbook.localize_tz() helper to make this more readable. Something like (untested):

def localize_tz(date, tz):
    if hasattr(tz, 'localize'):
        return tz.localize(date, is_dst=True)
    else:
        return date.replace(tzinfo=tz)

Or alteratively, a cbook.datetime_replace() that can handle the tz.

jklymak · 2018-11-18T18:14:35Z

I didn't make a cbook function because I can't see this being applicable outside of dates.py. But otherwise, thanks for the suggestion

timhoffm · 2018-11-18T18:54:23Z

The solution with a private method is equally fine.

timhoffm

Looking at the code, you always do a replace followed by _localize_tz. In that context a _date_replace migh even be better. Less clutter in the calling code and for non-pytz even a bit faster since you don't replace twice.

def _date_replace(date, tzinfo=None, **kwargs):
    """
    Drop-in replacement for datetime.replace() with support for pytz timezones.
    """
    if hasattr(tzinfo, 'localize'):
        date = date.replace(tzinfo=None, **kwargs)
        return tzinfo.localize(date, is_dst=True)
    return date.replace(tzinfo=tzinfo, **kwargs)

But I won't hold up if you want to stay with _localize_tz.

timhoffm · 2018-11-18T19:04:02Z

lib/matplotlib/dates.py

@@ -1409,13 +1419,22 @@ def tick_values(self, vmin, vmax):
        ymin = self.base.le(vmin.year) * self.base.step
        ymax = self.base.ge(vmax.year) * self.base.step

-        ticks = [vmin.replace(year=ymin, **self.replaced)]
+        vmin = vmin.replace(year=ymin, **self.replaced)
+        print('vmin before', vmin)


Debug print?

Suggested change

print('vmin before', vmin)

timhoffm · 2018-11-18T19:04:16Z

lib/matplotlib/dates.py

+        vmin = vmin.replace(year=ymin, **self.replaced)
+        print('vmin before', vmin)
+        vmin = _localize_tz(vmin, self.tz)
+        print('vmin after', vmin)


Debug print?

Suggested change

print('vmin after', vmin)

jklymak · 2018-11-18T21:39:13Z

Fair enough, that's an easy refactor....

timhoffm · 2018-11-18T21:45:40Z

lib/matplotlib/dates.py

@@ -1426,7 +1441,9 @@ def autoscale(self):
        ymin = self.base.le(dmin.year)
        ymax = self.base.ge(dmax.year)
        vmin = dmin.replace(year=ymin, **self.replaced)
+        vmin = vmin.astimezone(self.tz)


Is this acutally different from he above cases?

Ummm, hmmm, not sure this was properly tested....

pganssle

I think we worked everything out on this one, yeah? I think this can be merged.

jklymak · 2019-01-14T21:29:15Z

Rebased. Note two reviews...

timhoffm · 2019-01-20T20:31:10Z

The other reviewer does not have write access. Do we count this as a full review?

jklymak · 2019-01-20T20:35:43Z

No not really. Just hoping that someone else would take a quick look on the strength of the existing reviews and merge.

FIX: add check for py3.5 to fail if naive datetime TST: make pytz yearlylocator test

lumberbot-app · 2019-01-28T20:13:16Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.0.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 00d71cebb3f3e1eed5774b8942ebc9607961a61e

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #12678: FIX: properly set tz for YearLocator'

Push to a named branch :

git push YOURFORK v3.0.x:auto-backport-of-pr-12678-on-v3.0.x

Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR #12678 on branch v3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

jklymak mentioned this pull request Oct 30, 2018

Off-by-one bug in annual axis labels when localized time crosses year boundary #12675

Closed

jklymak added status: work in progress status: needs tests labels Oct 30, 2018

jklymak force-pushed the fix-yearlocator-tz branch from 245039b to 1d5b5dc Compare October 30, 2018 21:30

jklymak removed status: needs tests status: work in progress labels Oct 30, 2018

jklymak force-pushed the fix-yearlocator-tz branch from 1d5b5dc to b8207cb Compare October 30, 2018 21:42

jklymak added this to the v3.1 milestone Oct 30, 2018

jklymak added the topic: date handling label Oct 30, 2018

jklymak modified the milestones: v3.1, v3.0.x Oct 31, 2018

jklymak mentioned this pull request Oct 31, 2018

Travis error is MacOS image tolerance of 0.005 for test_constrained_layout.py::test_colorbar_location #12433

Closed

jklymak force-pushed the fix-yearlocator-tz branch 2 times, most recently from dadeae6 to 34cd842 Compare October 31, 2018 15:28

timhoffm previously approved these changes Nov 1, 2018

View reviewed changes

tacaswell reviewed Nov 4, 2018

View reviewed changes

pganssle reviewed Nov 4, 2018

View reviewed changes

jklymak commented Nov 4, 2018

View reviewed changes

jklymak force-pushed the fix-yearlocator-tz branch from 34cd842 to a6a96e3 Compare November 4, 2018 18:07

timhoffm reviewed Nov 18, 2018

View reviewed changes

jklymak force-pushed the fix-yearlocator-tz branch 2 times, most recently from 49b5d55 to 478ea3d Compare November 18, 2018 18:13

timhoffm reviewed Nov 18, 2018

View reviewed changes

jklymak force-pushed the fix-yearlocator-tz branch from 478ea3d to b5af9a9 Compare November 18, 2018 21:38

timhoffm approved these changes Nov 18, 2018

View reviewed changes

jklymak force-pushed the fix-yearlocator-tz branch 2 times, most recently from 0478bc1 to b244b3d Compare November 19, 2018 03:17

pganssle approved these changes Dec 28, 2018

View reviewed changes

jklymak force-pushed the fix-yearlocator-tz branch from b244b3d to 5ec912f Compare January 14, 2019 21:27

jklymak added 2 commits January 21, 2019 10:42

FIX: properly set tz for YearLocator

3219aa6

FIX: add check for py3.5 to fail if naive datetime TST: make pytz yearlylocator test

FIX: use localize instead of astimezone

d8fcd82

jklymak force-pushed the fix-yearlocator-tz branch from 5ec912f to d8fcd82 Compare January 21, 2019 18:43

tacaswell merged commit 00d71ce into matplotlib:master Jan 28, 2019

lumberbot-app bot added the status: needs manual backport label Jan 28, 2019

jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Jan 28, 2019

Merge pull request matplotlib#12678 from jklymak/fix-yearlocator-tz

7768522

jklymak added a commit to jklymak/matplotlib that referenced this pull request Jan 28, 2019

Backport PR matplotlib#12678: FIX: properly set tz for YearLocator

32d4164

jklymak added a commit to jklymak/matplotlib that referenced this pull request Jan 28, 2019

Backport PR matplotlib#12678: FIX: properly set tz for YearLocator

7ae4a36

jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Jan 28, 2019

Backport PR matplotlib#12678: FIX: properly set tz for YearLocator

48e839e

dstansby pushed a commit that referenced this pull request Jan 29, 2019

Backport PR #12678: FIX: properly set tz for YearLocator (#13308)

9f6cf2b

QuLogic removed status: needs review status: needs manual backport labels Feb 28, 2019

FIX: properly set tz for YearLocator #12678

FIX: properly set tz for YearLocator #12678

Conversation

jklymak commented Oct 30, 2018

PR Summary

PR Checklist

QuLogic commented Oct 30, 2018

jklymak commented Oct 30, 2018

jklymak commented Oct 30, 2018

jklymak commented Oct 30, 2018 • edited Loading

jklymak commented Oct 31, 2018

jklymak commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Nov 4, 2018

tacaswell commented Nov 4, 2018

jklymak commented Nov 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

pganssle commented Nov 4, 2018

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

jklymak commented Nov 18, 2018

timhoffm commented Nov 18, 2018

timhoffm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Nov 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle left a comment

Choose a reason for hiding this comment

jklymak commented Jan 14, 2019

timhoffm commented Jan 20, 2019

jklymak commented Jan 20, 2019

lumberbot-app bot commented Jan 28, 2019

jklymak commented Oct 30, 2018 •

edited

Loading

pganssle Nov 4, 2018 •

edited

Loading

timhoffm left a comment •

edited

Loading