Picklable figures #1020
Picklable figures #1020
Conversation
|
Update: I have just tried pickling a Basemap instance and associated figure and it has all gone swimmingly. This could be a very valuable time saver for creation of high resolution maps for a fixed location, from a batch job for instance (@jswhit will probably be most interested by this). |
|
Basemap instances were designed to be picklable, so that high-res instances that take a long time to create could be saved. The basemap hires.py example shows how much of a speed up you can get from this. |
I did not know that. Thanks Jeff. Please ignore my eureka moment which has already been handled by Basemap :-) . |
|
Glad to know someone finds it useful - I don't think anyone but me has ever used that feature. |
| return new_figure_manager_given_figure(num, figure) | ||
|
|
||
|
|
||
| def new_figure_manager_given_figure(num, figure): |
There was a problem hiding this comment.
Did anyone have any better ways of doing this rather than splitting up of new_figure_manager (which I still need to do for all of the backends)? It seems like a pattern that would pop up frequently if one were producing Figures without the plt.figure function, so I am surprised I couldn't find anything to "attach" a pre-created figure to the Gcf (and let it take care of making the manager for me).
There was a problem hiding this comment.
It seems that new_figure_manager (and the corresponding new_figure_manager_given_figure) is nearly identical in all the backends. Wouldn't it be better to extend the __init__() of FigureManager, so that it may be inherited?
There was a problem hiding this comment.
@akhmerov: Your suggestion sounds great, but sadly I don't agree that they are nearly identical in all the backends (I looked at qt4agg, wxagg and tkagg after reading your comment). That is not to say your inheritance suggestion does not deserve merit, it would be nicer to have new_figure_manager be a class method on a FigureManager class, but I'm not sure the benefit outweighs the cost of breaking backwards compatibility (it would be possible to put an alias to the class method in place of the current function, but that would mean there were two ways of doing the same thing...).
There was a problem hiding this comment.
@pelson: It is not entirely obvious that the backends qt4agg, wxagg and tkagg are so different. qt4 differs from tk by just classes it calls. In wx the difference is that there's a FigureFrame object, which has to be passed to a figure manager. Now __init__ of FigureFrame does what your new_figure_manager_given_figure does, and it also creates a figure manager. However at the same time, both __init__ of FigureManagerWx and FigureFrameWx require almost identical information, so it doesn't seem so crazy if e.g. FigureManagerWx initialized FigureFrameWx. This would also make sense given that FigureManager is the central object in other backends. Overall: I realize that for keeping backwards compatibility one would need to use an alias, and that it is not immediately obvious how to implement the proper behaviour, however it seems that the current state of backends is a mixture of OO code with weird naming scheme (why does one need backends.tkagg.FigureManagerTkAgg instead of backends.tkagg.FigureManager?), and copy-paste-modify non-OO code. This is a pain to interface to: for example IPython needed to hack into the code, to implement their own backend. I think that temporary existence of two ways of doing the same thing is a relatively low cost for having one of these ways clean.
There was a problem hiding this comment.
I can see you have a real passion to sort this out @akhmerov :-) . Would you be willing to submit a pull request against my branch (or even mpl/master) with some of your suggested changes and we can talk it through in-line? I do not propose you modify all backends at this point, perhaps just the 3 we have discussed.
There was a problem hiding this comment.
Yeah, indeed I have a real passion, only limited time. Is there any time after which it would become too late? Is it ok if it takes me around a month?
There was a problem hiding this comment.
Is it ok if it takes me around a month?
To base it on this branch that would be too long. There is a freeze planned for the 20th of August and I would like to get this branch in in the next week or so.
Having said this, the change I am proposing here is additive and not a major rework, so I am comfortable in making this change even with the knowledge that a better (larger) rework is on the cards in the future.
There was a problem hiding this comment.
Ok, I guess then I'll just do this independently and probably after the freeze.
|
I suspect that if I follow the polar examples, I could get the mplot3d stuff to work with this feature? |
Yeah. I was hoping to get some feedback on what I have done, before adding the testing machinery (and the necessary debugging from the pickle library if you come across something which doesn't pickle). Given there has been little comment, I will take that as a good thing and will look to adding tests and updating all of the backends soon-ish. |
| def __getinitargs__(self): | ||
| # note: __getinitargs__ only works for old-style classes | ||
| # means that the color cycle will be lost. | ||
| return (self.axes, self.command) |
There was a problem hiding this comment.
Shouldn't we be moving to new style classes anyway? Any harm in updating the class to new style (and then using __getstate__ instead?)
There was a problem hiding this comment.
If your happy for me to do that in this PR, then I will.
|
Looks good. The nested class thing is a complication with pickling that I wasn't aware of. I'm a bit concerned that those writing custom projections need to understand that complexity. Maybe it's better to not use nested classes for this (and update the "custom projection" docs accordingly?) I presume it's possible to have non-nested classes, but keep a reference to the class within the projection class, so as to not break backward compatibility, i.e.: |
Good idea. I didn't do it originally primarily for the backwards compatibility issue (plus size of review was a factor). Do you want me to do that in this PR on in a follow on one? |
|
I don't have a strong preference as to whether the nested class thing is resolved here or in a follow on -- but it would nice to not have to explain the peculiarities of pickles to those writing custom projections -- it's nicer to have everything "just work" in the obvious way. |
|
We should have a regression test for this. Something that
|
|
On a high level, what changes need to occur for other backends to support figure pickling? Great effort, by the way @pelson. This is something I've wanted for a while. |
|
Figure pickling does not involve any backend code. The key is that one must re-attach a figure to a backend, hence my addition of a |
|
One other comment (and I know you're still working on this) -- it would be nice to have an example that shows how to create, pickle and restore a figure. |
…and geo axes known so far)
| @@ -266,6 +266,13 @@ def __init__(self, *args): | |||
| self._cid = 0 | |||
| self._func_cid_map = {} | |||
|
|
|||
| def __getstate__(self): | |||
| return {} | |||
|
Ok. I'm making some good progress with this. I think the test coverage is good. Its highlighted some issues with some pretty fundamental artists (colorbars, legends and I want to make a couple more changes re the nested transforms in polar, but other than that, I would be happy to have this merged and subsequently fix other issues (colorbar and legend pickling for instance). Is that a suitable plan of attack? |
|
I think that's a fine plan of attack if that means that colorbar and legend pickling don't work but otherwise things that used to work are not broken (which I assume is the case, just asking for clarity). |
|
I would be happy with marking this as an experimental feature. |
|
@WeatherGod: Agreed. I have updated the whats new to reflect this. |
…lorbar/legend/geoaxes).
|
That last commit was a bit noisy due to the moving of the nested classes. It was a copy and paste job, and I have verified all the tests still pass. |
|
Ok. I am comfortable with this PR now. I was quite surprised how easy it was to apply the changes to all of the backends, and how similar that code was throughout (as @akhmerov says: there is definitely a lot of room for improvement there). I will look into the colorbar / legend support in the next few days hopefully and submit those as bug fixes (whether we decide to merge them is another matter). |
|
@pelson: I think this is a great feature. I'm trying to suss out the benefits of having it go in incomplete (without the colorbar and legend support) vs. waiting for it to be complete. I think I'd prefer it to be complete for the release. Do you think colorbar and legend support is a lot of additional work? |
|
@mdboom, I agree--this is a major new feature, but I don't think it should be in a release without colorbar and legend support. Releasing it that way would be just asking for a bunch of pointless email traffic and user confusion. Better to reserve it for 1.3 if necessary. |
| # check to see if the figure has a manager and whether it is registered | ||
| # with pyplot | ||
| if self.canvas is not None and self.canvas.manager is not None: | ||
| manager = self.canvas.manager |
There was a problem hiding this comment.
Will you need to add a manager to each backend's constructor method? It works well in pyplot, but using the object-oriented interface
import pickle as p
from matplotlib.backends.backend_pdf import FigureCanvasPdf as fc
from matplotlib.figure import Figure
fig = Figure()
can = fc(fig)
ax = fig.add_subplot(1, 1, 1)
ax.plot([1, 2, 3], [1, 2, 3])
fig.savefig('plot.pdf')
fout = open('pickled_fig.pkl', 'wb')
p.dump(fig, fout)
fout.close()throws an error: AttributeError: 'FigureCanvasPdf' object has no attribute 'manager'
Knowing the manager at construct-time means probing for the current backend, which fits in nicely with #1125.
There was a problem hiding this comment.
Er, I think I meant the Figure's constructor method, not the backend.
|
This is AWESOME, by the way. I just ran a little example using :D |
|
Firstly, thanks @dmcdougall and others for trying this out, its helpful to get all of your feedback. @efiring and @mdboom: It is inevitable that there will be features missing whenever this is first released. I have considered adding a format in the Because this is a completely new feature without any backwards incompatible changes, I would be comfortable slipping this in to the first RC with a week or so to spare, if anybody is uncomfortable then the alternative is to accept that this feature will not be available until v1.3. I make that to be a deadline of merging this before the 10th of September, else it will not get shipped just yet. Agreed? |
|
@pelson, I agree that with something like this, it can be good to get in the core, which you have now, and then use smaller PRs to add coverage. My main concern is getting into a situation where users expect everything to work, when common components don't work. If we did not have a release coming up, this would not be a problem. We could make an exception to the "feature freeze", and merge (with or without a rebase) what you have now. Then if colorbar and legend support can be added by rc1, and if everything looks OK, the feature can be added to "What's New". If not, it could be left in place, but with a note in the FAQ saying "pickle support is an underway project, but don't expect it to work until 1.3." "What's New" could also include a statement that internal changes have been made to lay the foundation, but it is not ready for use. |
|
I'm going to spend some time on this in the next day or so, so hopefully that should clarify the situation, and then we can make a decision whether to put this in v1.2.x or not. |
|
@pelson: Sounds good to me. Thanks again for your work on this. I know the IPython guys are very much looking forward to this. |
|
I have addressed @dmcdougall 's valuable feedback in the new PR (and added a test for his code). This PR is now obsolete |
I have been working on the ability to pickle figures (as mentioned in #323).
I have done quite a lot of testing of this (pyplot figures, non-pyplot figures, polar axes, contours, pcolormeshes etc.) but it is inevitable that there will be some types of plots that simply do not work at this stage (I would like to know about those).
There was a little (backwards compatible) shuffle in the way pyplot creates its figures which will need applying to all backends, and some unit tests will need adding, but other than that, I think this is in a form where it is worth getting other developers eyes on the code.
Any questions, please ask.