Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: clabel manual spacing was incorrect#9989
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
67a45dc toec24f95Compare| @@ -177,7 +177,7 @@ def test_contour_manual_labels(): | |||
| x, y = np.meshgrid(np.arange(0, 10), np.arange(0, 10)) | |||
| z = np.max(np.dstack([abs(x), abs(y)]), 2) | |||
| plt.figure(figsize=(6, 2)) | |||
| plt.figure(figsize=(6, 2), dpi=200) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why didn't this make the test images bigger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How big size wise is the new image? Would we get the same effect going down?
While you are regenerating the manual contour label test images can you also remove the text? |
@tacaswell Is this really the best test to remove text? |
d9ea329 to3901966Comparelib/matplotlib/contour.py Outdated
| @@ -575,6 +575,7 @@ def add_label_near(self, x, y, inline=True, inline_spacing=5, | |||
| # 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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a comment about units?
From the conversion we are getting pts out of theget_label_width andcalc_label_rot_and_inline is expecting pixels (and the doc strings seem to agree).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because implicit units are hard 😈 and we used to default to 80dpi on the screen so subtle things like this are easy to miss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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...
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. |
Sorry to keep asking you to re-re-generate the images. Moving them to the 'mpl20' style would also be good (remove jet!). |
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. |
3901966 to934b936CompareJust 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. |
934b936 to815601dCompareYes, sorry, I thought it was done on a per-file basis, but I figured out how to do it per-test |
Uh oh!
There was an error while loading.Please reload this page.
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