Fix manual contour label positions on sparse contours #1865
Conversation
Prior to this, contour labels were positioned at the nearest contour vertex, which in some cases (e.g., straight contours) could be quite distant from the desired location. This centers the label on the closest point on contour itself (using linear interpolation between vertices).
|
In the images above, the blue dots are the coordinates passed to the The script to generate these images is: import numpy as np
from matplotlib import pyplot as pt
import matplotlib as mpl
pt.figure(figsize=(6,2))
x,y = np.meshgrid(np.arange(0,10), np.arange(0,10))
z = np.max(np.dstack([abs(x),abs(y)]), 2)
cs = pt.contour(x,y,z)
pts = np.array([(1.5,3.0), (1.5,4.4), (1.5,6.0)])
pt.clabel(cs, manual=pts)
pt.scatter(pts[:,0], pts[:,1])
pt.savefig("contour_test.png") |
|
This is a nice piece of work. We're trying to clean up the code to PEP8 standards, so would you mind doing things like putting spaces after commas + two new lines after main level objects etc. etc.? Also, I appreciate that this is for manual contour label positioning, but can you conceive of an automatable test for this code? Finally, would you be able to add a section to the whats_new.rst file in the documentation. Thanks @rephorm |
|
I've updated both my new code and the other code in contour.py to remove any warnings the pep8 script gives, and added a section to the whats_new.rst file. |
|
Correct me if I am wrong, but wouldn't this patch also improve things for the automatic contour labeling (which has always been a pain)? If so, maybe we could find a degenerate case where the automatic contour labeling would be fixed by this and use that as a test? |
|
You can supply a list of points to the manual parameter, so setting up the test isn't too hard. I'm just not sure what the right metric for the test is. If you just want to require pixel-identical output for the given input in the future, then that wouldn't be hard to do. |
|
Ah, that would work too. Just simply utilize the regular image comparison framework that we have for everything else to produce the image in the example you gave originally. The image comparisons are designed to have a little bit of tolerance, and should be able to fail on the "before" image. We should make sure that it fails on the old behavior, of course. |
|
I added a simple test based on the script above. |
| @@ -137,3 +137,15 @@ def test_contour_shape_invalid_2(): | |||
| ax.contour(x, y, z) | |||
| except TypeError as exc: | |||
| assert exc.args[0] == 'Input z must be a 2D array.' | |||
|
|
|||
|
|
|||
| @image_comparison(baseline_images=['contour_manual_labels']) | |||
There was a problem hiding this comment.
contour_manual_labels_pdf.png and contour_manual_labels_svg.png are re-createable and do not need adding (I think there is a lack of documentation regarding this, so I can see why you've done it).
|
Yeah. The docs at http://matplotlib.org/devel/testing.html say to copy everything from result_images/test_foo/. |
| @@ -674,6 +686,64 @@ def labels(self, inline, inline_spacing): | |||
| paths.extend(additions) | |||
|
|
|||
|
|
|||
| def _find_closest_point_on_leg(p1, p2, p0): | |||
| '''find closest point to p0 on line segment connecting p1 and p2''' | |||
There was a problem hiding this comment.
Can you make these triple double quotes, """?
|
@rephorm This is a fantastic piece of work. Thanks for the patch! |
|
@dmcdougall Both of those choices were merely for consistency with existing code. I've update the entire file to use triple-double quotes and np.inf as suggested. |
|
@rephorn: Coding standards a bit new to matplotlib relative to its age, so there's a bit of a frustrating "do what I say, not what I do" feeling to it at the moment. It's slowly getting worked through, though. ;) |
|
@rephorm You did the right thing. As @mdboom points out, we are in a transitional phase of making our codebase PEP8 compliant, and since I noticed the triple single quotes I thought it'd be better to nip it in the bud. Thanks for propagating the change over the whole file, it's much appreciated. Anybody have any reservations? If not I'll push the shiny green button. |
|
@dmcdougall, @mdboom I understand. I just figured I'd explain myself. Glad to hear things are becoming more standardized! |
|
Looks good to me! |
Fix manual contour label positions on sparse contours
Prior to this, contour labels were positioned at the nearest contour
vertex, which in some cases (e.g., straight contours) could be quite
distant from the desired location (and occasionally not even on the
correct contour).
This centers the label on the closest point on contour itself (using
linear interpolation between vertices).
Old:
New:
