ENH: Change default Autodatelocator *interval_multiples* by jklymak · Pull Request #9801 · matplotlib/matplotlib

jklymak · 2017-11-16T17:22:39Z

PR Summary

OK, AutoDateLocator returned "not-nice" tick labels:

So like a dummy, I went and tried to implement better ticks that snapped to the tick intervals (10 minutes in this case). Then this morning I reread the code and found that lo-and-behold, it has a flag for this (interval_multiples=False)

I don't see the rationale behind the default being interval_multiples=False. Other than that it will break some existing tests, and its a breaking change. The resulting ticks look far better, and are much more amenable to panning. Tick locators are obscure enough that I never use them except in extremis, so expecting normal users to change options in them just to get something that looks somewhat rational is not putting our best foot forward.

PR Checklist

Regularize day intervals to include first of month
Has Pytest style unit tests
Code is PEP 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

jklymak · 2017-11-16T18:31:13Z

As a further suggestion for discussion, I'd also consider changing the interval for 14 days from a strict 14 days to first of month, 15th of month. We could consider regularizing all the daily intervals to be sure they fall on month boundaries.

The above fails the obvious image comparison tests...

jklymak · 2017-11-17T20:23:39Z

Is their appetite for changing the default formatters as well, particularly for dates:

I'd strip repeated entries. i.e. for "Feb 01", "Feb 08", "Feb 15", I'd strip "Feb" from the second two labels. i.e. only show month name when day == 1. Similarly, only show years when month == 1 (and day==1). That would make tick labels much more compact.

To make that work properly, the rcParams for the date formaters (i.e. rcParams['date.autoformat.day']) may need to change their meaning, so this would be a breaking change, the problem being that we would need to know how to parse the strings to strip information, and allowing the user to input arbitrary formatting information would make that hard.

tacaswell · 2017-11-18T22:18:49Z

I am open to this so long as there is clear and easy instructions on how to get old behavior back.

jklymak · 2017-11-19T01:03:31Z

I'll work on this.

I was also thinking a tutorial on how to label axes would be useful. I think that info is in a bunch of examples all over the place, but not explained systematically. @tacaswell I only hesitate because I keep hearing rumours that you have a documentation plan. If so, I can wait for that, and help contribute to that.

tacaswell · 2017-11-19T18:12:25Z

There is a plan to make a plan. In any outcome, the more content we have up front the less we have to write later (even if we re-organize or re-word it a bit).

If matching the current API is too annoying, making a new formatter / locator (but not using them by default yet) is a reasonable course of action as well.

jklymak · 2018-03-24T20:31:27Z

OK, this PR makes interval_multiples the default. It also changes the tick locator for months to be 1 and 15 if the 14-day locator is called for.

timhoffm · 2018-03-26T00:01:44Z

-                byranges[i] = self._byranges[i][::interval]
+                if i == 2 and interval == 14:
+                    # special case for monthday: Just tick 1 and 15:
+                    byranges[i] = range(1, 16, 14)


Could this not simply be [1, 15] to make it more clear?

dopplershift · 2018-03-26T19:21:33Z

 def test_auto_date_locator():
    def _create_auto_date_locator(date1, date2):
-        locator = mdates.AutoDateLocator()
+        # we prob. should eventually have a test w/ interval_multiples=True


I think this comment is out of date.

dopplershift · 2018-03-26T19:21:53Z

 # FreeType, and then use that to build the ft2font extension.  This
 # ensures that test images are exactly reproducible.
-#local_freetype = False
+local_freetype = True


This looks extraneous.

dopplershift · 2018-03-26T19:22:26Z


            if self._byranges[i] and self.interval_multiples:
-                byranges[i] = self._byranges[i][::interval]
+                print(self._byranges[i])


print() needs to go before merging.

efiring · 2018-07-03T01:14:26Z

                            5000, 10000, 20000, 50000, 100000, 200000, 500000,
                            1000000]}
+        if interval_multiples:
+            self.intervald[3] = [1, 2, 4, 7, 14, 21]


Instead of [3] do you mean [DAILY]?

efiring · 2018-07-03T01:17:25Z

I agree with the basic idea here of using "nice" values by default. It would be good to get this into 3.0.

jklymak · 2018-07-03T03:18:17Z

Reviews addressed, squashed and rebased, API note added (that may need CI to test if it rendered correctly).

efiring

Looks good.

efiring · 2018-07-03T12:31:12Z

                            5000, 10000, 20000, 50000, 100000, 200000, 500000,
                            1000000]}
+        if interval_multiples:
+            self.intervald[DAILY] = [1, 2, 4, 7, 14, 21]


Perhaps just curiosity, but why have this substitution of 4 for 3 in this case, and not in general? And isn't 21 a rather strange option? It is inconsistent with the docstring.

To clarify, the 21 option is present in the current code as well as in the line cited above, so the inconsistency predates your PR.

Yeah, I don't know why the 21 is in there. I left it in as possibly some weird edge case, but I don't see how it'd happen.

3 vs 4?

3: 1, 4,..., 28, 31, 1
4: 1, 5, ... 25, 29, 1

So except for Feb leap years it avoids an awkward cross-month transition.

added a couple of comments...

jklymak · 2018-07-03T17:49:05Z

This is ready for a second review.... Relatively simple change that I think makes the date locators much better...

Also make monthly byranges be 1 and 15

jklymak · 2018-07-04T19:08:22Z

squashed...

dstansby added the topic: date handling label Nov 16, 2017

dstansby added this to the v2.2 milestone Nov 16, 2017

afvincent added the API: default changes label Nov 16, 2017

dstansby mentioned this pull request Nov 23, 2017

YearLocator should prefer ticks at the turn of the decade #9838

Closed

jklymak mentioned this pull request Dec 11, 2017

General poor default formatting of datetimes on plot x-axis #9978

Closed

jklymak modified the milestones: v2.2, v3.0 Jan 10, 2018

jklymak added the status: work in progress label Mar 18, 2018

jklymak mentioned this pull request Mar 19, 2018

ENH: new date formatter #10841

Merged

6 tasks

jklymak force-pushed the fixdateticklocator branch 2 times, most recently from 07eeb57 to 1d5fa6e Compare March 24, 2018 20:30

jklymak added status: needs review and removed status: work in progress labels Mar 24, 2018

jklymak changed the title WIP: FIX: Change default Autodatelocator *interval_multiples* ENH: Change default Autodatelocator *interval_multiples* Mar 24, 2018

jklymak removed the status: needs review label Mar 24, 2018

jklymak force-pushed the fixdateticklocator branch 2 times, most recently from be9c155 to 3356826 Compare March 25, 2018 04:12

timhoffm reviewed Mar 26, 2018

View reviewed changes

jklymak force-pushed the fixdateticklocator branch from 3356826 to e090ff4 Compare March 26, 2018 03:38

jklymak added status: work in progress and removed status: work in progress labels Mar 26, 2018

dopplershift reviewed Mar 26, 2018

View reviewed changes

efiring reviewed Jul 3, 2018

View reviewed changes

jklymak force-pushed the fixdateticklocator branch 2 times, most recently from b755888 to 44c0bbb Compare July 3, 2018 03:16

efiring approved these changes Jul 3, 2018

View reviewed changes

jklymak added the status: needs review label Jul 3, 2018

jklymak force-pushed the fixdateticklocator branch 2 times, most recently from 1e609db to 7c994b8 Compare July 3, 2018 23:29

FIX: Change default Autodatelocator *interval_multiples*

0337883

Also make monthly byranges be 1 and 15

jklymak force-pushed the fixdateticklocator branch from 7c994b8 to 0337883 Compare July 4, 2018 19:08

timhoffm approved these changes Jul 4, 2018

View reviewed changes

timhoffm merged commit d80c9fb into matplotlib:master Jul 4, 2018

jklymak deleted the fixdateticklocator branch July 4, 2018 20:07

QuLogic removed the status: needs review label Jul 5, 2018

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 mentioned this pull request Jan 14, 2019

Unevenly spaced ticks in plot_date() #13183

Closed

Uh oh!

Conversation

jklymak commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

jklymak commented Nov 16, 2017

Uh oh!

jklymak commented Nov 17, 2017

Uh oh!

tacaswell commented Nov 18, 2017

Uh oh!

jklymak commented Nov 19, 2017

Uh oh!

tacaswell commented Nov 19, 2017

Uh oh!

jklymak commented Mar 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring commented Jul 3, 2018

Uh oh!

jklymak commented Jul 3, 2018

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jul 3, 2018

Uh oh!

jklymak commented Jul 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jklymak commented Nov 16, 2017 •

edited

Loading

efiring Jul 3, 2018 •

edited

Loading