Skip to content

Backend plt/gcf refactor #2624

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

Closed
wants to merge 36 commits into from

Conversation

tacaswell
Copy link
Member

This is an alternative to #2617. The idea is to factor Gcf out of the toolkit backends so that the manager classes can be reused in external gui applications.

These changes should be backwards compatible.

Added an example for Qt4 to show how to use the managers in an external program.

Currently only backend_bases, qt4 and qt4agg are done, but the other backends should be doable incrementally.

The relavent backends are (I think)

  • qt4/qt4Agg
  • gtk
  • gtk3
  • macosx
  • pdf (in PdfPages)
  • ps
  • tkAgg
  • webAgg
  • wx

This possibly should get a MEP.

@fariza
Copy link
Member

fariza commented Nov 29, 2013

@tacaswell why remove key_press_handler for just one method?
I understand that if you are coding your own app, you may not want it, but, still an important part to leave it out.

@fariza
Copy link
Member

fariza commented Nov 29, 2013

@tacaswell Why to leave new_figure_manager in the Gcf independent part?
For my taste it just looks awkward and is prone to confusion.
If you are coding your external app, an extra line is worth if it makes it clear.

@mdboom
Copy link
Member

mdboom commented Nov 29, 2013

I like the concept here, as illustrated by the new_figure_manager_easy_way method... I wonder if in the long term, we only want to show the easy method in the example (and maybe just have the new example replace the old one). Even though we'll have to continue to support the old API for some time, we generally removed "old" methods from the examples when they've been obsoleted. Is there any additional flexibility in the hard way that is missing from the new way?

While I see why it's an improvement, I'm concerned about the API breakage of moving backend_bases. There are external backends (the HTML5 one, for example) that import backend_bases and inherit from things in it. Now it is not only moved, but privatized. It doesn't seem necessary to do this to get done what you need to here, or maybe I'm missing something. We could add a backward compatibility layer here, but I think I'd prefer to just leave backend_bases.py where it currently is unless there's good reason to move it.

@tacaswell
Copy link
Member Author

key_press_handler isn't removed, it is just at the layer that knows about Gcf.

I'll add a commit re-arranging it so that everything but the close key get handled in _backend_bases.py

@fariza
Copy link
Member

fariza commented Nov 29, 2013

That's what I meant, removed from Gcf independent part.
At the same time, wouldn't it be better to move name num as kwarg (default to None) in FigureManagerBase as @efiring was suggesting?

@tacaswell
Copy link
Member Author

I left new_figure_manager in the Gcf independent because it is independent of Gcf and one of the goals of this is to make it easy to get graph windows in your application which makes the new_figure_manager very useful outside of pyplot

I am not sure on replacing the current example, I think it is good to keep an example of how to do the embedding at a low-level around (instead of telling people to go look at the source of FigureManager) because sometimes you want to build your application around a canvas.

backend_bases.py bulk-imports everything from backends/_backend_bases.py so I think it should look 100% reverse compatible from the outside. I used leading _ because it was easy and I am bad at naming things.


from matplotlib.backends.qt4_compat import QtCore, QtGui
import numpy as np
from matplotlib.backends._backend_qt4agg import (FigureCanvasQTAgg,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we want to have examples importing from private modules, or using private variables. My interpretation of the leading underscore is, "for internal use only, not part of the public API, please don't use this, but if you do, it is at your own risk."

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 like suggestions of how to rename things, I went with _ because it was easy. What I really want to do is make a module backend_*_pyplot which contains the Gcf related code, but that would break badly break the API. backend_*_clean seems misleading, backend_*_stateless seems too long, any abbreviation seems too cryptic. Should have made clear that I don't think this is near being ready to merge.

@mdboom
Copy link
Member

mdboom commented Dec 2, 2013

Ah, I see what you're going with the backend_bases.py/_backend_bases.py change now. Github hid the diff on that because it thought it was too large... ;)

So, you're plan is to separate out the pyplot-related things into separate modules? Maybe it would be helpful to be explicit about those design principles -- I'd probably better understand what's happening here (or just point me to the discussion, if it already happened elsewhere and I missed it).

@tacaswell
Copy link
Member Author

The discussion of this is rather scattered across a number of issues/PRs related to the tabbed gui between @fariza and my self.

The basic idea here is that we already have in the codebase everything we need to smooth over the differences between different tool kits, but the code is mixed up with plt/gcf which makes re-using it hard. The first step is to get the gui stuff into modules that are clean from any of the global state in Gcf. For backwards compatibility I renamed the clean modules and via import made sure the public interface doesn't change. (Eric has pointed out my naming scheme is awful).

The next step is to rip the FigureManager classes apart into the window level code and the canvas level code and for each backend provide widget that takes care of the boiler plate for setting up and connecting the canvas and building a figure on it's self. These widgets can then be used by 3rd party programs to easily embed a graph (pythonxy provides such a widget for QT, but I have not looked at it because it is GPL and I don't know how serious people are about a clean room implementation) and to build the tabbed windows in a very straight forward way with minimal code replication.

  • backend_xx.py (sticky tapes Gcf logic on to FigureManager, TabbedFigureManager) used by pyplot
    • imports from backend_xx_clean.py (all three of these class are ready to embed using pure OO interface)
    • FigureManager <- uses FigureWidget to provide window with single figure in it
    • TabbedFigureManager <- uses FigureWidget to tabbed window with multiple figures
    • FigureWidget <- uses canvas_xx to provide a widget with a Figure built and ready to go

There is very little new code that needs to be written for this, we just need to re-arrange what is already there.

@tacaswell
Copy link
Member Author

even if this PR goes no where, the pep8 commits should be cherry-picked out.

@tacaswell
Copy link
Member Author

re-based to so it merges nicely again.

@tacaswell
Copy link
Member Author

As a side question, gkt3 + ipython does not seem to work on linux and closing/deleting figures seems to raise some extraneous exceptions. Are these known issues?

self.canvas.destroy()
if self.toolbar:
self.toolbar.destroy()
self.__dict__.clear() # Is this needed? Other backends don't have it.
Copy link
Member

Choose a reason for hiding this comment

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

This is the line that causes the strang exceptions when closing and/or deleting figures.

Copy link
Member

Choose a reason for hiding this comment

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

Seems to me that is safe to remove that line.

Copy link
Member Author

Choose a reason for hiding this comment

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

should that line be removed?

@fariza
Copy link
Member

fariza commented Dec 4, 2013

I am not a big user of ipython but last time I checked there was no mode for Gtk3. But to be fair, I didn't even try more than that.

@tacaswell
Copy link
Member Author

@fariza Can you check that I have not made a hash of the gtk3 code? Does this structure make sense? Can you verify that what I am suggesting with the FigureWidget level of classes actually makes sense in GTK3?

@tacaswell
Copy link
Member Author

@mdboom What can I do to make reviewing this easier?

@mdboom
Copy link
Member

mdboom commented Dec 4, 2013

I see the issue with Gtk3 even on master -- I'm looking at making a separate bugfix for that now, then I'll come back and look at this.

Thanks for all this hard work -- this is a tricky bit of refactoring.

@mdboom
Copy link
Member

mdboom commented Dec 4, 2013

See #2646 for a fix for the Gtk3 issue.

@mdboom
Copy link
Member

mdboom commented Dec 4, 2013

A thought occurred to me: Am I correct that the reason for splitting the backend* files is so that users who don't want pyplot functionality don't inadvertently import pyplot and enable it? Perhaps that could be instead addressed through importing within functions rather than at the top level... It seems that should work for everything with the exception of ShowBase, which as a base class needs to be imported at the top level. But maybe that could be addressed by using something other than inheritance for those Show classes. Then there's far less moving around of code as well. I admit that doesn't necessarily make it as clear to users when they are using pyplot, but we can probably address that (somewhat) through better documentation and examples about how to do this sort of embedding without pyplot.

Does that make any sense, or am I missing something?

@WeatherGod
Copy link
Member

Importing within functions? @mdboom, are you sipping the eggnog a bit early
this year? Whatever problems we have, I can be sure that doing imports
within functions is not the way to solve it.

@mdboom
Copy link
Member

mdboom commented Dec 4, 2013

@WeatherGod: Any better ideas? I understand there's an overhead, but show and new_figure_manager are not frequently called functions deep in inner loops. It's a completely legitimate way to reduce import-time dependencies.

@tacaswell
Copy link
Member Author

The other gotcha is all of the destroy methods of the gui need to have call backs to Gcf to remove the figure when the window is closed (as does the default key bindings and the gtk clean-up-the-main-loop code). That is the main reason I split the files up like this.

There are ways of pushing the call-back registration up the stack to pyplot, to require less code movement, but doing it that way would break reverse compatibility.

new_figure_manager actually knows nothing about Gcf, it get called by Gcf with an integer and the returned value is stashed by Gcf.

@WeatherGod
Copy link
Member

It isn't the overhead that I am worried about, it is just really bad form
to do that. It makes static analysis more difficult, and nearly nobody
looks for imports anywhere but the top of the file. PEP8 says that imports
should be at the top of the file. Lastly, it changes the API of the
module. While it isn't a good idea to depend on these things, people do do
it.

@fariza
Copy link
Member

fariza commented Dec 4, 2013

Keep in mind, the main objective of this splitting is to be able to use the "manager classes" in other gui applications.
At the moment it seems that it diverted to getting rid of pyplot.
Is that the same?

Personally the only thing that bothers me from pyplot is that the way it handles the gtk/gtk3 destroy.

@fariza
Copy link
Member

fariza commented Dec 4, 2013

I mentioned this before,
Why not just get Gcf outside pyplot?
@efiring argued that Gcf was at the heart of pyplot, but the same happens with the tracking of current axes, and that is inside figure.

Please illuminate me, but I don't see any bad side effect of having Gcf outside pyplot. The right imports will make it transparent.

@tacaswell
Copy link
Member Author

In some sense, Gcf is already outside of pyplot (it is in _pylab_helpers.py) and is imported into pyplot.py, but as Eric says it is the heart of the pyplot functionality in that it keeps track of what the current figure is (and keeps a list of all the figures you have open). That list (and the associated machinery on destroy) is the global state that this PR is about separating from the backend code. Once this is done, we can add parameters to the FigureManager constructors to do things like set the window titles to arbitrary strings.

Figure keeping track of it's current axes is a purely local and different than the global/singleton nature of Gcf. gca is part of the internal state of Figure and 3rd party code is indifferent to that state. The global gca is implemented by asking the current Figure what it's current axes is (plt.gca() -> plt.gcf().gca()). At worst keep track of the current axes makes the Figure objects slightly bigger than strictly needed, but I don't think we want to worry about that.

@fariza
Copy link
Member

fariza commented Dec 5, 2013

@tacaswell I don't know how to do a PR against your PR (git is messing with my brain), but your example is not working
Please add FigureManagerQTAgg = FigureManagerQT at the end of _backend_qt4agg.py

@tacaswell
Copy link
Member Author

@fariza If you have a local copy of my branch, make your changes on the tip and then push your branch to your github repo (just like making changes against master).

The PR interface in github has issues with large networks. The advice they gave us was to edit the URL by hand to point the PR against the correct repo/branch.

I fixed this by removing the FigureManagerQTAgg from the example. That class got burned out of existence (#2629).

@fariza
Copy link
Member

fariza commented Dec 5, 2013

Thanks for the git advice, I will try later.

I know that FigureManagerQTAgg doen't exist, but I think uniformity in terms of naming is important.
Taking your example, it doesn't make sense to import
FigureCanvasQTAgg and FigureManagerQT from _backend_qt4agg.py
This stateless backend should expose either:

  • FigureManager and FigureCanvas
  • FigureCanvasQTAgg and FigureManagerQTAgg
  • FigureCanvasQT and FigureManagerQT

But not a mix.
Personally I find that all backends (stateless or not) should expose FigureManager and FigureCanvas, so the user know always what to import.

@tacaswell
Copy link
Member Author

I would push a head with what you want to do and I will deal with the complications for my stuff later.

Sorry for dropping the ball so badly on this.

@OceanWolf
Copy link
Member

@tacaswell In #2759, @fariza suggested in response to one of my comments that a global event handling framework could become the subject of a future MEP. As at the moment it seems very self-contained within FigureCanvasBase.

What do you think, should I begin writing one? As events form a major part the side covered by this refactor, I thought it the best place to ask.

@tacaswell
Copy link
Member Author

If you are interested, then yes. Also look at the discussion in #2986 about how to handle pick events.

@tacaswell tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015
@OceanWolf
Copy link
Member

@tacaswell Do you have a plan for the breaking of this up into smaller commits? Or even an MEP document to look which outlines this? I would like to have a look at this, but over 13k changes in the codebase looks a bit daunting to start reading through...

@tacaswell
Copy link
Member Author

most of those lines are just copies as I split files up.

The outline of this is that nothing in backend* should import pyplot_helpers. The problem with changing that is that it breaks API so the solution I was going for was to split the files in half, one part was 'clean' and could be used for embedding in a way that does not ever import Gcf and the other half which maintains the current API.

Getting back to this has been on my to-do list for over a year now....

This should be split up, one PR to do backend_bases.py and one PR per backend after that.

@OceanWolf
Copy link
Member

Perhaps something like the Navigation refactor, i.e. one PR for backend_bases.py plus one backend as an example, then the other backends go separate PRs?

I have very little time to look at this right now, but I would imagine the way to do this refactor would just mean splitting out the pure backend logic from the general mpl logic, in master we have the FigureManagerGTK3 backend doing both:

self.set_window_title("Figure %d" % num)

Here it combines the mpl logic title = "Figure %d" % num, with the backend logic self.set_window_title(title).

As we have ToolbarGTK3 to do a toolbar and FigureCanvasGTK3 to abstract the canvas drawing, so too I think we need FigureWindowGTK3 to abstract away the windowing parts. Then we can have FigureManager dealing with just the mpl logic of how these abstracted backend components work together.

@fariza
Copy link
Member

fariza commented Feb 20, 2015

I am starting to implement the MEP23 https://github.com/matplotlib/matplotlib/wiki/Mep23 and I have conflicts with Gcf and general pyplot stuff (I can hack my way around that but not clean).

@tacaswell just a crazy idea, what do you think if I implement the MEP23 in a completely new set of MultiFigureManagers, that don't include any pyplot stuff. So no backward compatibility problem, and if somebody wants to use it, they will have to adhere to whatever new interactive mode we define.

@OceanWolf
Copy link
Member

@tacaswell Anyway, sounds simple enough, I shall work on getting a PR on this done in the next few hours :).

@tacaswell
Copy link
Member Author

@OceanWolf Great!

Just to be clear, I am ecstatic that you are taking over this project.

@OceanWolf
Copy link
Member

@tacaswell Just to make it clear, I don't know much about the various backends, so I will put all my focus on refactoring just GTK3, in doing so it may look very GTK centric in its base approach, which I then expect to polish out later.

@OceanWolf
Copy link
Member

@tacaswell Getting there, just finished the 2nd class :).

One question though, I see the method FigureManagerBase.show_popup, though I see no reference to it anywhere. Shall I just remove this from the refactor?

@tacaswell
Copy link
Member Author

We have to be very careful about changing public API. It either needs to stay or to go through a proper deprecation cycle.

@OceanWolf
Copy link
Member

@tacaswell I make completely new classes, so by remove, I mean not putting them in the new one... I think I shall leave it out for now, and make a comment about it in the PR.

@tacaswell
Copy link
Member Author

👎 on completely new classes for this core stuff unless they match the old API exactly. There is too much existing code (particularly for embedding). I don't want to maintain two version of code that do the same thing, that way lies madness confusion and angry users.

I am now very confused about what you are working on.

@OceanWolf
Copy link
Member

DW, I have backwards compatibility as my middle name ;).

@tacaswell
Copy link
Member Author

Replaced by MEP27 and #4143

@tacaswell tacaswell deleted the backend_plt_refactor branch April 17, 2015 21:06
@leycec leycec mentioned this pull request Dec 13, 2018
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.

6 participants