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
Conversation
… manually produce ugly spikes in the contours. The problem was that screen coordinates were being passed as data coordinates when the contour was split to add room for the label. Incorporate my fix and miili's matplotlib#2806 change
This exercises with input in both display and data units.
This bug took far to long for me to understand..... |
fixes #2475 |
@tacaswell, as you noted, all this is hard to follow; but it looks plausible. Would it be worth adding a few brief comments to the code, while you are in the middle of it? |
I has been a while sense I looked at this so it would take me a while to sort out what was going on again. I don't follow what you are asking about |
My point is that |
@tacaswell, Since it looks like there will be no more 1.3.x releases, would you rebase this against master, please? I think it is important that this go into 1.4. |
I agree it should go in 1.4. I left it against 1.3.x just in case someone makes a good case that we should do a 1.3.2 and merging is easier for my brain than cherry-picking merges. |
@@ -576,10 +576,7 @@ def add_label_near(self, x, y, inline=True, inline_spacing=5, | |||
# be a vertex in the path. So, if it isn't, add a vertex here | |||
paths = self.collections[conmin].get_paths() |
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.
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
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 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.
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.
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.
- added some in-line comments to explain what is going on
@WeatherGod Turns out Path objects don't know about transforms. @efiring Added some explanatory comments |
@@ -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 |
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 think this comment is not helping; the nearby docstring and the code are readable as they are.
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.
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.
disp_units = [(216, 177), (359, 290), (521, 406)] | ||
data_units = [(-2, .5), (0, -1.5), (2.8, 1)] | ||
|
||
CS.clabel() |
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.
Would it be better to leave out this clabel
call, so we could see the labels specified directly below?
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 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.
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 would also note that the location of the added labels does not overlap with where the default labels are placed.
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.
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.
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 will try to get to this today
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.
The problem is that the call to clabel
sets up a bunch of data-structure that the add_label_near
depends on.
So, I have no idea what is going on with travis. 3.2 decided to install again and both of the 2.x decided they don't pass pep8 (but the 3.x's do). |
BUGFIX: This change fixes #2475, where contour labels added manually
produce ugly spikes in the contours. The problem was that screen
coordinates were being passed as data coordinates when the contour was
split to add room for the label.
Incorporate @bwkeller fix and @miili's #2806 change
See long comment on #2818 for why this is the correct fix.