FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: clabel manual spacing was incorrect by jklymak · Pull Request #9989 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

FIX: clabel manual spacing was incorrect - #9989

Merged
dstansby merged 2 commits into
matplotlib:masterfrom
jklymak:Fix-clabel-manual-space
Dec 14, 2017
ConversationCommits (2)ChecksFiles changed
Merged

FIX: clabel manual spacing was incorrect#9989
dstansby merged 2 commits into
matplotlib:masterfrom
jklymak:Fix-clabel-manual-space

Conversation

jklymak commented Dec 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Fixes #9988. This fails some tests, but I'd argue it fails them correctly. Certainly the situation in #9988 is not acceptable.

The tests weren't catching this error because the test figure dpi is 72, so the tests look OK.

Before

After

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant

jklymak mentioned this pull request Dec 12, 2017
Closed
jklymak added this to the v2.2 milestone Dec 12, 2017
jklymak force-pushed the Fix-clabel-manual-space branch from 67a45dc to ec24f95 Compare December 13, 2017 00:18
tacaswell reviewed Dec 13, 2017
View reviewed changes
Comment thread lib/matplotlib/tests/test_contour.py
z = np.max(np.dstack([abs(x), abs(y)]), 2)

plt.figure(figsize=(6, 2))
plt.figure(figsize=(6, 2), dpi=200)

tacaswell Dec 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why didn't this make the test images bigger?

jklymak Dec 13, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

umm, I think because the printed version doesn't care about figure dpi? There must be a way to change the test image dpi.... I'll check Thanks

jklymak Dec 13, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed

tacaswell Dec 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How big size wise is the new image? Would we get the same effect going down?

tacaswell commented Dec 13, 2017

Copy link
Copy Markdown
Member

While you are regenerating the manual contour label test images can you also remove the text?

jklymak commented Dec 13, 2017

Copy link
Copy Markdown
Member Author

@tacaswell Is this really the best test to remove text?

jklymak force-pushed the Fix-clabel-manual-space branch from d9ea329 to 3901966 Compare December 13, 2017 02:05
tacaswell reviewed Dec 13, 2017
View reviewed changes
Comment thread lib/matplotlib/contour.py Outdated
# Get label width for rotating labels and breaking contours
lw = self.get_label_width(self.labelLevelList[lmin],
self.labelFmt, self.labelFontSizeList[lmin])
lw *= self.ax.figure.dpi / 72.0

tacaswell Dec 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add a comment about units?

From the conversion we are getting pts out of the get_label_width and calc_label_rot_and_inline is expecting pixels (and the doc strings seem to agree).

jklymak Dec 13, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just copied the same line in the vanilla label below. I’ll readily admit I don’t understand why this stuff is all such a mess.

tacaswell Dec 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because implicit units are hard 😈 and we used to default to 80dpi on the screen so subtle things like this are easy to miss.

jklymak Dec 13, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, I get it now. Its a little funny this uses the heuristic of 0.6 * fontsize * len(str) to get the width. I thought we had better ways to do that, but maybe we pre-draw first to do that. Anyway, that can be for a future day...

tacaswell commented Dec 13, 2017

Copy link
Copy Markdown
Member

The text will only pull off the tick labels, the contour labels will still be there. The less places we have the tick labels the less our exposure to the freetype issues are.

tacaswell commented Dec 13, 2017

Copy link
Copy Markdown
Member

Sorry to keep asking you to re-re-generate the images. Moving them to the 'mpl20' style would also be good (remove jet!).

jklymak commented Dec 13, 2017

Copy link
Copy Markdown
Member Author

Ok hang on if I remove jet then I have to redo all the images. Did you want that? I can easily remove the other text decorations.

jklymak force-pushed the Fix-clabel-manual-space branch from 3901966 to 934b936 Compare December 13, 2017 03:15

tacaswell commented Dec 13, 2017

Copy link
Copy Markdown
Member

Just re-do the ones you have already touched. It isn't a huge deal either way, but we will eventually want to move away from 'classic' for the tests.

jklymak force-pushed the Fix-clabel-manual-space branch from 934b936 to 815601d Compare December 13, 2017 03:24

jklymak commented Dec 13, 2017

Copy link
Copy Markdown
Member Author

Yes, sorry, I thought it was done on a per-file basis, but I figured out how to do it per-test

dstansby merged commit 49d5ced into matplotlib:master Dec 14, 2017
QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
jklymak deleted the Fix-clabel-manual-space branch March 5, 2019 16:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell approved these changes
dstansby dstansby approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.2.0

Development

Successfully merging this pull request may close these issues.

Contours are not removed correctly when using clabel with manual

4 participants

Back | FazBrowse Home | New Git URL