Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: This change fixes #2475, where contour labels added manually #2843

Merged
merged 4 commits into from Jul 10, 2014
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 18 additions & 6 deletions lib/matplotlib/contour.py
Expand Up @@ -562,26 +562,38 @@ def add_label_near(self, x, y, inline=True, inline_spacing=5,
exact for labels at locations where the contour is
straight, less so for labels on curved contours.
"""

# if 'do the default thing' use the axes data transform
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 comment is not helping; the nearby docstring and the code are readable as they are.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, the docstring could be made clearer:
"Add a label near the point (x, y). If transform is None (default), (x, y) is in data coordinates; if transform is False, (x, y) is in display coordinates; otherwise, the specified transform will be used to translate (x, y) into display coordinates."

Maybe something like that would be clearer.

if transform is None:
transform = self.ax.transData

# if transform is 'falsey' don't do any transform (this is a
# really awful API but is what we have)
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this comment does more harm than good, to my eye.

if transform:
# The point of this transform is to get the location
# of label position into screen units
Copy link
Member

Choose a reason for hiding this comment

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

And this comment is also excessive...

x, y = transform.transform_point((x, y))

# find the nearest contour _in screen units_
Copy link
Member

Choose a reason for hiding this comment

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

...but this one is good.

conmin, segmin, imin, xmin, ymin = self.find_nearest_contour(
x, y, self.labelIndiceList)[:5]

# The calc_label_rot_and_inline routine requires that (xmin,ymin)
# be a vertex in the path. So, if it isn't, add a vertex here

# grab the paths from the collections
paths = self.collections[conmin].get_paths()
Copy link
Member

Choose a reason for hiding this comment

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

what if the path object from the collection has a different transform than what we are expecting? Otherwise, this all makes sense, albeit it could use some more commenting

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the assumption is that the points in the path are in data space as iirc everything returned by find_nearest_contour is working in screen space. The bug was in the case were you wanted to specify the location of the label in screen-units, but when the location never got converted back to data-space units when adding a point to the path which is what generated the huge spikes.

I think the issue you are bring up is if points in the path are not specified in data units so instead of self.ax.transData.inverted() in should be paths[segmin].get_transform().inverted()? Presumably that transform should be passed back in on line 585/582 when the new path is created.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is exactly what my concern is. The transforms framework can have a lot of redundant parts, which can mask incorrect usage. We shouldn't assume that we know the transform of the path object, that is known only by the path object itself, so we should use it here.

Admittedly, my brain does mini-explosions reading through some of these code parts because I easily lose track of the coder's intent and expectation.

lc = paths[segmin].vertices
if transform:
xcmin = transform.inverted().transform([xmin, ymin])
else:
xcmin = np.array([xmin, ymin])
# grab the correct segment
active_path = paths[segmin]
# grab it's verticies
lc = active_path.vertices
# sort out where the new vertex should be added in the path's data
# units
xcmin = self.ax.transData.inverted().transform_point([xmin, ymin])
# if there isn't a vertex close enough
if not np.allclose(xcmin, lc[imin]):
# insert new data into the vertex list
lc = np.r_[lc[:imin], np.array(xcmin)[None, :], lc[imin:]]
# replace the path with the new one
paths[segmin] = mpath.Path(lc)

# Get index of nearest level in subset of levels used for labeling
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
30 changes: 29 additions & 1 deletion lib/matplotlib/tests/test_contour.py
@@ -1,5 +1,5 @@
import numpy as np

from matplotlib import mlab
from matplotlib.testing.decorators import cleanup, image_comparison
from matplotlib import pyplot as plt

Expand Down Expand Up @@ -183,6 +183,34 @@ def test_given_colors_levels_and_extends():
plt.colorbar()


@image_comparison(baseline_images=['contour_test_label_transforms'],
extensions=['png'], remove_text=True)
def test_labels():
# Adapted from pylab_examples example code: contour_demo.py
# see issues #2475, #2843, and #2818 for explanation
delta = 0.025
x = np.arange(-3.0, 3.0, delta)
y = np.arange(-2.0, 2.0, delta)
X, Y = np.meshgrid(x, y)
Z1 = mlab.bivariate_normal(X, Y, 1.0, 1.0, 0.0, 0.0)
Z2 = mlab.bivariate_normal(X, Y, 1.5, 0.5, 1, 1)
# difference of Gaussians
Z = 10.0 * (Z2 - Z1)

fig, ax = plt.subplots(1, 1)
CS = ax.contour(X, Y, Z)
disp_units = [(216, 177), (359, 290), (521, 406)]
data_units = [(-2, .5), (0, -1.5), (2.8, 1)]

CS.clabel()
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to leave out this clabel call, so we could see the labels specified directly below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember there was some reason that I I had this call first, but I don't remember what it was. The test shows the sum of the labels plus the ones added manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would also note that the location of the added labels does not overlap with where the default labels are placed.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but from simply looking at the plot, or the code, I can't easily see which are which. Since you have verified that everything is in its place, this doesn't matter much; the test will still do its job.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to get to this today

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that the call to clabel sets up a bunch of data-structure that the add_label_near depends on.


for x, y in data_units:
CS.add_label_near(x, y, inline=True, transform=None)

for x, y in disp_units:
CS.add_label_near(x, y, inline=True, transform=False)


if __name__ == '__main__':
import nose
nose.runmodule(argv=['-s', '--with-doctest'], exit=False)