Fix for overlapping datetime intervals by shadidsh · Pull Request #10779 · matplotlib/matplotlib · GitHub

PR Summary

Fix for issue #7712

This PR addresses the overlapping date intervals that can still be found. I noticed PR #8251 just changed the hard coded data table, yet it still resulted in overlaps when dealing with minutely changes. Without changing the data table, I used rcParam's figsize and fontsize to make a conservative estimation of characters that can fit per inch.

Although the solution requires more information I believe this makes it less dependent on future changes to the table. I have modified all image tests required, added coverage, and added to next_api_changes to inform users that image outputs have changed. Thanks in advance for any feedback on this PR.

PR Checklist

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

… tests cases to fit, added next_api_changes

…fix-issue-7712

I think you need to get the width of the axes, not the figure. I'd also expect 10-pt font characters to be about 72 points-per-inch / 10 points-per-character = 7.2 characters-per-inch wide.

Font size generally refers to the height, not the width (i.e., 10pt monospace, sans, serif are not all the same width).

Understood, but IMO, the new ticks are too spread out. 1) because using the wrong width to work over and 2) because the fontwidth is too wide.

I agree with that; almost all the test images that changed appear mostly okay before.

In general I'm not in favour of this PR, simply because it seems that it creates not enough labels for the general case and the determination of the spacing seems to be rather arbitrary. Adding this either as a different WideSpreadAutoDateLocator or via an argument AutoDateLocator(spread="generous") to the existing one seems to make more sense to me.

Else, I would argue that the spreading needs to take into account the actual fontsize (not the one from rcParams) and also the rotation of the labels - but that may be rather difficult, because those are not know before drawtime, right?

I think tick locators are determned at draw time, so thats OK, but you are correct that it should use the actual fontsize of the tick labels, not the one from rcParams!

I think the trouble with estimating the actual size would require the Text objects. Due to the order in which we use the locators in Axis's iter_ticks() there will be no text when we determine the number of ticks, which is the main cause of this issue. Also, any suggestions on retrieving the width of the axis?

Rotation:
When dealing with the xAxis (and yAxis) class which is what we have access to in DateLocator, I was not able to retrieve the rotation of the labels, I believe because it does not set rotation before determining the number of intervals.

Font size of tick labels:
As mentioned above, I believe the tick labels are empty because it decides the number of ticks before it gets the tick labels.

Not enough labels:
I think this issue goes hand in hand with rotation, as in when is it possible to determine if its rotated or not, in other words we need to know its going to be rotated before we place the number of ticks.

AutoDateLocator with parameter:
This is preferable but our parameter would go up to Axis, in particular:
Tick uses converter.axisinfo(self.units, self) which calls AutoDateLocator. So if i'm not mistaken the parameter (spread="generous") would be a property of the Artist class.

I will try to implement DateLocator with this parameter.

…or.spacing to rcparams' as an option

To make some progress and ensure previous image tests are preserved, I added rcParams["autodatelocator.spacing"] and I am using the axis's label for font and axis's figure for figsize. However I do have some concerns:

@jklymak is the width of the axes calculated before those intervals are placed? if so I am not able to determine how from the axis. I also have same question to get width from the label's Text, not sure which property to investigate.

@shadidsh the width of the axes can be got from ax.get_position(original=False). Its in figure-normal co-ordinates, so you should use a transform to get to inches.

… into fix-issue-7712

There seem to be several test image changes with imperceptible differences, leading me to think these images should not be changing at all.

…of axes

I removed the unnecessary images, after changes to the PR I did not check them out properly.

The code now uses the axes width transformed from the axes's position. Please note the reason I added the parameter to RcParams is because rotation of label at this point does not seem to be set, thus spacing would be problematic. Any feedback and suggestions would be greatly appreciated, thanks.

I think this should all be wrapped in if spacing:

I don't get why you are using getattr for all these. If these attributes don't exist, you might as well error here because you weren't passed an axis...

If we raise exceptions it would break existing tests using mock classes that might have aaxis attributes as none.

Which tests? I don't understand what a test would be doing down in here w/o the axes being set (for instance).

for example test_dates.py::def test_auto_date_locator(), this uses dummy axis to specifically check values returned from autodatelocator. It has a few none axis attributes

https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_dates.py#L320

OK, well I'd just check if whichever one of these is None and set spacing=False if that happens

I'm pretty sure the ticks have a fontsize associated with them beyond the label fontsize. THats the fontsize you should querry, and I'm pretty sure it exists.

again, this should only happen if spacing is on.

??? I don't understand why you only break if apply_spread=True here.

When users have rotated labels we shouldn't apply spread, it applies too much spacing. Unfortunately this is why i added to Rcparams in the first place

But you need "interval" to be correct for byranges, below.

Maybe autodatelocator.generousspacing : False True/False would be a cleaner API?

Some comments provided.

I'm 50/50 on the PR as a whole. The issue is that most of the time dates on the xaxis are too long. What'll happen here is that there will just be too few ticks. The real solution is making the ticklabels shorter (i.e. #10841) and rotating the ticklabels.

@jklymak Thanks so much for the feedback, when i get the chance I will add the fixes for experience so I am fine if the PR is closed after the update.

@shadidsh thanks so much for the PR. Sorry that it didn't make it in, but it was a good idea. We are curretnly discussing tick-related issues, with an idea to completely overhaul them in 3.0. Now that you have some development experience under your belt, please feel free to contribute to that discussion. @timhoffm is leading it, though for the life of me, I can't remember or find what the issue is.

@timhoffm can we open a modern issue, w/ you as the author, to keep track of tick-related things...

@shadidsh hopefully we see more of you! Thanks again.

shadidsh added 5 commits March 9, 2018 18:07

overlap tick dates

2e8184b

avoid floating numbers as intervals

36dd933

finished changes in AutoDateLocator, added and modified several image…

19bb1f0

… tests cases to fit, added next_api_changes

Merge branch 'master' of git://github.com/matplotlib/matplotlib into …

4159670

…fix-issue-7712

comment for minutely intervals

0af2379

jklymak reviewed Mar 14, 2018

View reviewed changes

updated solution to use dimensions from axis, and added autodatelocat…

9b65519

…or.spacing to rcparams' as an option

remove extra spread var for now

cc2c074

shadidsh added 4 commits March 16, 2018 04:35

pep 8 fix

ad15851

pep 8 compliance

5325ff2

pep8 compliance

242b4b2

Merge branch 'fix-issue-7712' of https://github.com/shadidsh/matplotlib …

c5afe20

… into fix-issue-7712

shadidsh added 2 commits March 21, 2018 01:27

revert unnecessary expected images, update dates to use actual width …

a7a0a3f

…of axes

fixed comments and removed extra unrequired image change

202ed25

jklymak reviewed Mar 23, 2018

View reviewed changes

shadidsh closed this Apr 11, 2018

jklymak added the topic: ticks axis labels label Apr 11, 2018

+                      if label is not None:
+                          font_ratio = label.get_fontsize() / 10
+                      else:
+                          font_ratio = rcParams['font.size'] / 10

+                                              ((num/interval) * datelen_ratio) <= maxwid)
+                              if (num <= interval * (self.maxticks[freq] - 1)):
+                                  if (apply_spread):
+                                      break

+                      font_ratio = (rcParams['font.size'])/10
+                      # a ratio of 10 date characters per inch is 'estimated'
+                      maxwid = rcParams["figure.figsize"][0] * 10

                       #  bysecond, unused (for microseconds)]
                       byranges = [None, 1, 1, 0, 0, 0, None]
+                      # Required attributes to get width from figure's normal points

                               byranges[i] = None
                               continue
+                          # Compute at runtime the size of date label with given format

               #date.autoformatter.second   : %H:%M:%S
               #date.autoformatter.microsecond   : %M:%S.%f
+              #autodatelocator.spacing : tight ## if generous, add extra spacing to avoid overlap, otherwise tight

Fix for overlapping datetime intervals by shadidsh · Pull Request #10779 · matplotlib/matplotlib · GitHub

Repository navigation

Fix for overlapping datetime intervals - #10779

Fix for overlapping datetime intervals#10779shadidsh wants to merge 13 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom shadidsh:fix-issue-7712shadidsh/matplotlib:fix-issue-7712Copy head branch name to clipboard

Conversation

shadidsh commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

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!

ImportanceOfBeingErnest commented Mar 14, 2018

Uh oh!

jklymak commented Mar 14, 2018

Uh oh!

shadidsh commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shadidsh commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak commented Mar 16, 2018

Uh oh!

efiring commented Mar 18, 2018

Uh oh!

shadidsh commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadidsh Mar 23, 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!

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!

Choose a reason for hiding this comment

Uh oh!

shadidsh Mar 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Mar 23, 2018

Uh oh!

shadidsh commented Mar 23, 2018

Uh oh!

jklymak commented Apr 11, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix for overlapping datetime intervals#10779
shadidsh wants to merge 13 commits into
matplotlib:masterfrom
shadidsh:fix-issue-7712

shadidsh commented Mar 14, 2018 •
edited
Loading

shadidsh commented Mar 14, 2018 •
edited
Loading

shadidsh commented Mar 16, 2018 •
edited
Loading

shadidsh Mar 23, 2018 •
edited
Loading

shadidsh Mar 23, 2018 •
edited
Loading