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

Fix suptitle #1663

Merged
merged 3 commits into from Jan 18, 2013
Merged

Fix suptitle #1663

merged 3 commits into from Jan 18, 2013

Conversation

dmcdougall
Copy link
Member

New version of #1276 but which targets v1.2.x and includes a test.

If there's no feedback I'll merge this in a day or two. I'd at least like @NelleV to check PEP8-ness, if you wouldn't mind :)

@@ -65,6 +65,13 @@ def test_gca():
assert_true(fig.gca(projection='rectilinear') is ax1)
assert_true(fig.gca() is ax1)

Copy link
Member

Choose a reason for hiding this comment

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

PEP8 compliance: you need two blank lines here.

@NelleV
Copy link
Member

NelleV commented Jan 16, 2013

I've underlined the two PEP8 problem. Else, the code looks good to me.

@dmcdougall
Copy link
Member Author

Wooo! A legitimate test failure!

@pelson Do you have any idea why that pickle test is failing? Looks like that title in the pickle baseline image is a suptitle, but the kwargs are still being set just as they were before this change. Perhaps the addition of a self._suptitle variable affects how the figure object gets pickled.

@pelson
Copy link
Member

pelson commented Jan 16, 2013

@dmcdougall - I'll have a butchers tomorrow at this for you. Has the result image gained a suptitle that wasn't there before?

@dmcdougall
Copy link
Member Author

@pelson Another travis downside: I can't access the failing image. I'll run the test locally and report back.

@dmcdougall
Copy link
Member Author

@pelson The produced image doesn't have a suptitle. Will look into it.

@dmcdougall
Copy link
Member Author

I've found the issue. Since the suptitle (a Text object) now persists in the Figure class, having remove_text=True in the image_comparison decorator for the test_pickle test removes the text when producing the resulting image.

There are a couple of ways to proceed:

  1. Remove remove_text=True, resulting in a persisting suptitle but potentially causing further failures because of text issues.

  2. Keep remove_text=True and remove the suptitle. There is an additional check inside test_pickle to ensure that figure labels are preserved on unpickling. Downside is a new baseline image has to be committed.

  3. Don't merge this pull request.

I think 3) is overly harsh. I'd prefer to execute option 2), but if there is a consensus for 1), then I'm fine with that too.

@WeatherGod
Copy link
Member

I am +1 on option 2.

@mdboom
Copy link
Member

mdboom commented Jan 16, 2013

Ditto for option 2.

@dmcdougall
Copy link
Member Author

Done.

t = self.text(x, y, t, **kwargs)
return t
if self._suptitle is not None:
self._suptitle.set_text('')
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 should call the .remove() method, rather than just making it a null Text object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or perhaps .update, and put the line below inside an else:?

Copy link
Member

Choose a reason for hiding this comment

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

Either way works. 😉

@dmcdougall
Copy link
Member Author

@pelson Better?

@pelson
Copy link
Member

pelson commented Jan 18, 2013

Yep. 👍 from me. @NelleV - are you happy with it?

@NelleV
Copy link
Member

NelleV commented Jan 18, 2013

LGTM: 👍 for merging

@dmcdougall
Copy link
Member Author

Ok. I'll clean up the commit history and merge this (py31 failure is a dud). Thanks.

dmcdougall added a commit that referenced this pull request Jan 18, 2013
@dmcdougall dmcdougall merged commit 527fd07 into matplotlib:v1.2.x Jan 18, 2013
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 23, 2013
(PR matplotlib#1505).

Tests added is matplotlib#1663 pass with this patch.
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.

None yet

5 participants