Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix for overlapping datetime intervals#10779

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

Closed
shadidsh wants to merge13 commits intomatplotlib:masterfromshadidsh:fix-issue-7712

Conversation

shadidsh
Copy link

@shadidshshadidsh commentedMar 14, 2018
edited
Loading

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

font_ratio = (rcParams['font.size'])/10

# a ratio of 10 date characters per inch is 'estimated'
maxwid = rcParams["figure.figsize"][0] * 10
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@ImportanceOfBeingErnest
Copy link
Member

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 differentWideSpreadAutoDateLocator or via an argumentAutoDateLocator(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 theactual 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?

@jklymak
Copy link
Member

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!

@shadidsh
Copy link
Author

shadidsh commentedMar 14, 2018
edited
Loading

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.

@shadidsh
Copy link
Author

shadidsh commentedMar 16, 2018
edited
Loading

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:

  1. @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'sText, not sure which property to investigate.

@jklymak
Copy link
Member

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

@efiring
Copy link
Member

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

anntzer reacted with thumbs up emoji

@shadidsh
Copy link
Author

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.

@@ -1314,11 +1332,34 @@ def get_locator(self, dmin, dmax):
# bysecond, unused (for microseconds)]
byranges = [None, 1, 1, 0, 0, 0, None]

# Required attributes to get width from figure's normal points
Copy link
Member

Choose a reason for hiding this comment

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

I think this should all be wrapped inif 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...

Copy link
Author

@shadidshshadidshMar 23, 2018
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

@@ -1328,11 +1369,24 @@ def get_locator(self, dmin, dmax):
byranges[i] = None
continue

# Compute at runtime the size of date label with given format
Copy link
Member

Choose a reason for hiding this comment

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

again, this should only happen if spacing is on.

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

Choose a reason for hiding this comment

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

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

Copy link
Author

@shadidshshadidshMar 23, 2018
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

But you need "interval" to be correct forbyranges, below.

@@ -352,6 +352,8 @@ backend : $TEMPLATE_BACKEND
#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
Copy link
Member

Choose a reason for hiding this comment

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

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

@jklymak
Copy link
Member

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.

@shadidsh
Copy link
Author

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

@jklymak
Copy link
Member

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@jklymakjklymakjklymak left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@shadidsh@ImportanceOfBeingErnest@jklymak@efiring@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp