Skip to content

Reverted a previous change to artist transform setting. Fixes legend bug. - #1176

Merged
mdboom merged 1 commit into
matplotlib:masterfrom
pelson:legend_subplot_fix
Sep 11, 2012
Merged

mdboom merged 1 commit into
matplotlib:masterfrom
pelson:legend_subplot_fix

Conversation

@pelson

@pelson pelson commented Aug 30, 2012

Copy link
Copy Markdown
Member

On master, the following code fails to put the markers in a legend in the correct place:

import matplotlib.pyplot as plt

plt.subplot(222)

plt.subplot(223)
plt.scatter(range(10), range(10), label='foo')
plt.legend()

plt.show()

It turns out this is a result of a previous pull request of mine (#1090). This PR fixes this regression (and adds a suitable test). I will milestone this for 1.2 as it is a pretty critical bug fix.

@WeatherGod

Copy link
Copy Markdown
Member

That's really strange. I would have considered the "t is not None" test as being superior to blindly setting the boolean to True. Maybe some code somewhere is fiddling around with the _transform attribute directly when it shouldn't?

@pelson

pelson commented Aug 30, 2012

Copy link
Copy Markdown
Member Author

That's really strange. I would have considered the "t is not None" test as being superior to blindly setting the boolean to True

Me too! The Artist._transformSet attribute is used by the Artist.is_transform_set method. The only place this is used is in axes.py and offsetbox.py. I believe the issue is in offsetbox, which checks is_transform_set and if it isn't, adds a generic transform (from offsetbox.DrawingArea.get_transform()). All this stuff is pretty deep in mpl, and I am a little hesitant to change some of this stuff, hence I reverted my own change.

@travisbot

Copy link
Copy Markdown

This pull request fails (merged 654b2922 into cf7618c).

@mdboom

mdboom commented Aug 30, 2012

Copy link
Copy Markdown
Member

Thanks for finding this and tracking this down. I think what's not clear is that a transform of None is set, that's equivalent to an identity transform, and should be treated as an explicit transform having been set as far as the offsetbox is concerned. I think this change is correct, though I haven't run it against the test suite to see if it produces additional failures.

@pelson

pelson commented Aug 30, 2012

Copy link
Copy Markdown
Member Author

though I haven't run it against the test suite to see if it produces additional failures

You were right to have your suspicions. I do get failures as a result of this. Back to the drawing board... might not find a solution to this until tomorrow.

@travisbot

Copy link
Copy Markdown

This pull request fails (merged 4965b5df into cf7618c).

@pelson

pelson commented Sep 7, 2012

Copy link
Copy Markdown
Member Author

Ok. I have solved this issue the proper way. I did a lot of reading of the legend code (phew, its not particularly easy to follow), found the problem (the transforms weren't being set correctly) and fixed a couple of typos.

Unfortunately, my editor had a setting enabled to fix the whitespace problems automatically, which adds a little bit of noise to this PR. It wouldn't be unreasonable to ask me to revert them, but I am keeping them for now as they do improve the overall readability of the code (and hence my sanity while I was debugging this).

@mdboom

mdboom commented Sep 7, 2012

Copy link
Copy Markdown
Member

The fancy_test fails for me due to text differences. I think either the freetype version needs to be restricted (not ideal), or the text needs to be changed to use characters that don't exhibit these differences.

imgur

@pelson

pelson commented Sep 10, 2012

Copy link
Copy Markdown
Member Author

or the text needs to be changed to use characters that don't exhibit these differences

@mdboom any advice on how best to go about this?

@mdboom

mdboom commented Sep 10, 2012

Copy link
Copy Markdown
Member

Maybe the best/easiest thing to do is to just take the text out -- the legend should layout good enough for the purposes of the test without it.

@mdboom
mdboom merged commit 3bfb6b8 into matplotlib:master Sep 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants