FIX: clabel manual spacing was incorrect #9989
Conversation
67a45dc to
ec24f95
Compare
| @@ -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.
Why didn't this make the test images bigger?
There was a problem hiding this comment.
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.
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 to
3901966
Compare
lib/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.
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).
There was a problem hiding this comment.
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.
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.
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 to
934b936
Compare
|
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. |
934b936 to
815601d
Compare
|
Yes, sorry, I thought it was done on a per-file basis, but I figured out how to do it per-test |
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