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

Nbagg enhancements #3552

Merged
merged 4 commits into from Sep 24, 2014
Merged

Nbagg enhancements #3552

merged 4 commits into from Sep 24, 2014

Conversation

pelson
Copy link
Member

@pelson pelson commented Sep 22, 2014

Extends the nbagg backend to support animation, improves some of the code, and fixes a focus issue when closing figures.

@pelson pelson added this to the v1.4.1 milestone Sep 22, 2014
@pelson
Copy link
Member Author

pelson commented Sep 22, 2014

@mdboom - just wanted to ping you on this because I've changed the behaviour such that by default you get a white background for the figure (as per savefig) rather than adhering to the patch's facecolor.

I've taken liberties with these enhancements against v1.4.1 purely because the nbagg backend is pretty experimental and these changes make the backend more user friendly. I'd be pretty disappointed to put them against v1.5.x/master, but it wouldn't take much argument to encourage me to do it, if anybody has major concerns.

Notice that I've added a UAT document - automated testing of the nbagg backend is hard. In the future I can imagine some limited mock unit-tests, but there will always be a need for a few integration tests. I was torn between putting the UAT in the backend code vs the testing folder - I can live comfortably with either, so again, if anybody has a preference, just let me know.

@pelson
Copy link
Member Author

pelson commented Sep 22, 2014

@mdboom - just wanted to ping you on this because I've changed the behaviour such that by default you get a white background for the figure (as per savefig) rather than adhering to the patch's facecolor.

Scratch that - I've changed my mind and decided that simply adding a rcParam which determines if the nbagg defaults to transparent is the easiest and most consistent approach.

@mdboom
Copy link
Member

mdboom commented Sep 22, 2014

Looks good. I don't have a strong argument about where the UAT should live, but maybe it deserves a mention in the developer docs about running the tests?

Is there a good reason why the rcParam figure.transparent couldn't be reused here?

@pelson
Copy link
Member Author

pelson commented Sep 22, 2014

Is there a good reason why the rcParam figure.transparent couldn't be reused here?

I don't believe that exists currently? :

>>> print(matplotlib.rcParams.find_all('transparent'))
nbagg.transparent: True
savefig.transparent: False

The main reason for that specific commit is to change the default from a gray figure patch to something more useful (either white or, better still, transparent) when using the nbagg backend. I didn't want to venture into the realm of changing defaults for all backends, but feel that the nbagg backend has an opportunity to avoid ugly gray edges from the outset.

@mdboom
Copy link
Member

mdboom commented Sep 22, 2014

savefig.transparent, not figure.transparent is what I meant. Sorry for the confusion. And I see what you're saying -- don't want to change the defaults for everything at this point.

if not interactive and manager in pylab_helpers.Gcf._activeQue:
pylab_helpers.Gcf._activeQue.remove(manager)

if not is_interactive() and manager in Gcf._activeQue:
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been de-indented? This reads as only operating on the last figure. If that is intentional, I think this should read as managers[-1]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep you're right.

@pelson
Copy link
Member Author

pelson commented Sep 23, 2014

Looks like the web agg framework is struggling with transparency actually - I've only just seen it, but I'd say that the renderer isn't being cleared properly between draws, and therefore is sending over images with ghosting type effects. I'll try to take a closer look soon, but I suspect the quick solution is to set the figure facecolor to white for now.

@pelson
Copy link
Member Author

pelson commented Sep 24, 2014

Looks like the web agg framework is struggling with transparency actually - I've only just seen it, but I'd say that the renderer isn't being cleared properly between draws

That was exactly it. I've now fixed the webagg backend to solve this issue, and as a result, the nbagg backend is now functioning as expected. I'm very happy with the result of this PR - it turns nbagg into a genuinely usable backend.

@pelson
Copy link
Member Author

pelson commented Sep 24, 2014

P.S. I've tested this with both Python 2.7 and Python 3.4 - both work as expected.

@tacaswell tacaswell merged commit aa583e4 into matplotlib:v1.4.x Sep 24, 2014
@tacaswell
Copy link
Member

Fixed pep8 issue locally, merged and pushed.

@mdboom
Copy link
Member

mdboom commented Sep 25, 2014

👍 Great stuff.

@pelson
Copy link
Member Author

pelson commented Sep 25, 2014

👍 Great stuff.

Except I broke the image diff updates - #3567 addresses the problem.


output = buff

if not self._force_full and not some_transparency:
Copy link
Member Author

Choose a reason for hiding this comment

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

This has been raised as a concern by @mdboom in #5419.

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

3 participants