NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

ENH: Change default Autodatelocator interval_multiples#9801

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

timhoffm merged 1 commit intomatplotlib:masterfromjklymak:fixdateticklocator

Jul 4, 2018

Merged

ENH: Change default Autodatelocator interval_multiples#9801

timhoffm merged 1 commit intomatplotlib:masterfromjklymak:fixdateticklocator

Jul 4, 2018

Conversation

Copy link

Member

jklymak commentedNov 16, 2017•
edited
Loading

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

dstansby added the topic: date handling label

Nov 16, 2017

dstansby added this to thev2.2 milestone

Nov 16, 2017

afvincent added the API: default changes label

Nov 16, 2017

Copy link

MemberAuthor

jklymak commentedNov 16, 2017

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

Copy link

MemberAuthor

jklymak commentedNov 17, 2017

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.

Copy link

Member

tacaswell commentedNov 18, 2017

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

Copy link

MemberAuthor

jklymak commentedNov 19, 2017

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.

Copy link

Member

tacaswell commentedNov 19, 2017

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.

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 thefixdateticklocator branch 2 times, most recently from07eeb57 to1d5fa6eCompare

March 24, 2018 20:30

Copy link

MemberAuthor

jklymak commentedMar 24, 2018

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

jklymak added status: needs review and removed status: work in progress labels

Mar 24, 2018

jklymak changed the titleWIP: 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 thefixdateticklocator branch 2 times, most recently frombe9c155 to3356826Compare

March 25, 2018 04:12

timhoffm reviewed

Mar 26, 2018

View reviewed changes

lib/matplotlib/dates.py Outdated

		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)

Copy link

Member

timhoffmMar 26, 2018

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

jklymakMar 26, 2018

Choose a reason for hiding this comment

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

Yep!

jklymak force-pushed thefixdateticklocator branch from3356826 toe090ff4Compare

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

lib/matplotlib/tests/test_dates.py Outdated

		@@ -319,7 +319,8 @@ def test_empty_date_with_year_formatter():

		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

Copy link

Contributor

dopplershiftMar 26, 2018

Choose a reason for hiding this comment

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

I think this comment is out of date.

dopplershift reviewed

Mar 26, 2018

View reviewed changes

setup.cfg.template Outdated

		@@ -13,12 +13,12 @@
		# set this to True. It will download and build a specific version of
		# FreeType, and then use that to build the ft2font extension. This
		# ensures that test images are exactly reproducible.
		#local_freetype =False
		local_freetype =True

Copy link

Contributor

dopplershiftMar 26, 2018

Choose a reason for hiding this comment

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

This looks extraneous.

dopplershift reviewed

Mar 26, 2018

View reviewed changes

lib/matplotlib/dates.py Outdated

		@@ -1338,7 +1341,11 @@ def get_locator(self, dmin, dmax):
		self._freq = freq

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

Copy link

Contributor

dopplershiftMar 26, 2018

Choose a reason for hiding this comment

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

print() needs to go before merging.

efiring reviewed

Jul 3, 2018

View reviewed changes

lib/matplotlib/dates.py Outdated

		@@ -1234,6 +1234,9 @@ def __init__(self, tz=None, minticks=5, maxticks=None,
		MICROSECONDLY: [1, 2, 5, 10, 20, 50, 100, 200, 500, 1000, 2000,
		5000, 10000, 20000, 50000, 100000, 200000, 500000,
		1000000]}
		if interval_multiples:
		self.intervald[3] = [1, 2, 4, 7, 14, 21]

Copy link

Member

efiringJul 3, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

efiring commentedJul 3, 2018

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

jklymak force-pushed thefixdateticklocator branch 2 times, most recently fromb755888 to44c0bbbCompare

July 3, 2018 03:16

Copy link

MemberAuthor

jklymak commentedJul 3, 2018

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

efiring approved these changes

Jul 3, 2018

View reviewed changes

Copy link

Member

efiring left a comment

Choose a reason for hiding this comment

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

Looks good.

lib/matplotlib/dates.py Outdated

		@@ -1234,6 +1234,9 @@ def __init__(self, tz=None, minticks=5, maxticks=None,
		MICROSECONDLY: [1, 2, 5, 10, 20, 50, 100, 200, 500, 1000, 2000,
		5000, 10000, 20000, 50000, 100000, 200000, 500000,
		1000000]}
		if interval_multiples:
		self.intervald[DAILY] = [1, 2, 4, 7, 14, 21]

Copy link

Member

efiringJul 3, 2018

Choose a reason for hiding this comment

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

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.