MEP27 Decouple pyplot from backends (refactoring Gcf out of backend code) - #4143
MEP27 Decouple pyplot from backends (refactoring Gcf out of backend code)#4143OceanWolf wants to merge 55 commits into
Conversation
Sorry, something went wrong.
|
@tacaswell Not finished yet, still some refactoring to go, just putting the PR for early feedback to catch things that I have perhaps missed. I have finished the first pass (out of 2, I hope), so it should all work. I have worked solely on the GTK3Cairo backend so far. All other backends should work as normal, though I haven't tested (and I still see no Travis here). |
Sorry, something went wrong.
|
Atm, I use the existing structure, i.e. using new_figure_manager to pass the classes we want to use. However I do like the way we get the new_figure_manager in matplotlib/backends/__init__.py and thought about getting the specific classes we want to use directly from there, something like: from matplotlib.backends import Window, Toolbar2, Canvasand in matplotlib.backends.__init__.py put: _temp = __import__('backend_'+backend, globals(), locals(), ['Window', 'Toobar2', 'Canvas'], -1)
Window = _temp.Window
Toolbar2 = _temp.Toolbar2
Canvas = _temp.Canvas |
Sorry, something went wrong.
|
I provisionally think this is a good idea. This deserves more attention than I can give it right now, sorry 😞 Would this method also provide a (sane) path to change which interactive backend you are using on the fly? I don't think it will, but make sure that this does not interfere with how we do saving (which is (more-or-less) by replacing the canvas on the Figure object and re-calling draw and then restoring the old canvas). |
Sorry, something went wrong.
|
Thank you, I shall push ahead then with the additions to matplotlib.backends.__init__.py (which I think looks a lot nicer, tucking all the backend specific code properly away behind the scenes); and the next Refactor pass. Perhaps not a sane path, (at least not right now), but I guess it will provide a saner path for later (as with this we get some clear structure to how the backends work). I have never tried to change backends and so don't know the current path, so I don't really know. I imagine that with the additions to __init__.py we can do away with the backend new_figure_manager function, just leaving it there for a deprecation cycle. |
Sorry, something went wrong.
|
@tacaswell Almost finished, just finishing the refactoring Gcf out of matplotlib.backend_bases.ShowBase, tough cookie, but I have managed it quite well I think, especially with respect to switching backends (I now have it with very few lines of code, to switch backends without having to close the previous backend -- if wanted --, the refactor enables it to magically take care of all of that :). I haven't touched the backend switching code, but have built in flexibility to add it later). One question arises though, should the ShowBase callable instance (aka backends.backend_xxx.show, get called from anywhere, or just pyplot/Gcf? I need to get an idea of the use-cases of this (Note I have refactored ShowBase as FigureManagerBase, but I use the old terms so that we both speak the same language). I just need to come up with a suitable place(s) to put the code. Having Gcf everywhere makes it difficult to figure out to what extent it should have a role ;). If show should only get called via Gcf, then I shall probably create a method Gcf.show_all() that calls the refactored show. |
Sorry, something went wrong.
|
I mean I feel 95% sure, that the backend show() callable-class should only get called by Gcf, just because afaik, we have no other way of tracking FigureManagers, so to show all figures, seems impossible. Okay, I think have talked myself round to believing my assumption as correct and will give it a try. If I have overlooked something here please let me know! |
Sorry, something went wrong.
|
Okay, so I have finished the 2nd refactor pass so open for comments, I have tested examples/slider_demo on both Gtk3Cairo which I have refactored, and on TkAgg which I haven't, and both work fine, so I feel happy! I did have problems with absolute imports due to __init__.py, but wrapping it in a function fixed that. Will feel good to get this landed as it will eradicate the backend hacks from #3652. Of course I imagine changes to WindowBase will most likely occur, but personally I like to learn as I go, when problems present themselves. Too many different backend mechanisms to study in one go. |
Sorry, something went wrong.
|
Yay Travis working again, but boo failing, did I introduce an error here, or does this represent a problem with the testing framework? Looking at the log for 2.6 I get the following, and the IOError: Conversion command failed: makes it look like a testing framework bug to me (the pickling bug comes from figure.Figure I think, I shall investigate). ======================================================================
ERROR: test suite for <class 'matplotlib.tests.test_pickle.test_complete'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/plugins/multiprocess.py", line 788, in run
self.setUp()
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/plugins/multiprocess.py", line 770, in setupContext
super(NoSharedFixtureContextSuite, self).setupContext(context)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/util.py", line 470, in try_run
return func()
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 134, in setup_class
cls._func()
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/tests/test_pickle.py", line 193, in test_complete
assert_not_equal(plt._pylab_helpers.Gcf.figs, {})
AssertionError: {} == {}
======================================================================
ERROR: matplotlib.tests.test_patheffects.test_collection.test
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/case.py", line 197, in runTest
self.test(*self.arg)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 51, in failer
result = f(*args, **kwargs)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 186, in do_test
self._tol, in_decorator=True)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/compare.py", line 310, in compare_images
expected = convert(expected, True)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/compare.py", line 192, in convert
converter[extension](filename, newname)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/compare.py", line 118, in convert
raise IOError(msg)
IOError: Conversion command failed:
======================================================================
FAIL: matplotlib.tests.test_patheffects.test_collection.test
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/case.py", line 197, in runTest
self.test(*self.arg)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 51, in failer
result = f(*args, **kwargs)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 196, in do_test
'(RMS %(rms).3f)'%err)
ImageComparisonFailure: images not close: /home/travis/build/matplotlib/tmp_test_dir/result_images/test_patheffects/collection.png vs. /home/travis/build/matplotlib/tmp_test_dir/result_images/test_patheffects/collection-expected.png (RMS 31.876)
======================================================================
FAIL: matplotlib.tests.test_patheffects.test_collection.test
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/nose/case.py", line 197, in runTest
self.test(*self.arg)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 51, in failer
result = f(*args, **kwargs)
File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 196, in do_test
'(RMS %(rms).3f)'%err)
ImageComparisonFailure: images not close: /home/travis/build/matplotlib/tmp_test_dir/result_images/test_patheffects/collection_pdf.png vs. /home/travis/build/matplotlib/tmp_test_dir/result_images/test_patheffects/collection-expected_pdf.png (RMS 38.061)
|
Sorry, something went wrong.
|
Okay, now that Travis gives the thumbs up to this PR, I now see it time to start pushing out to other backends. The strategy for this:
|
Sorry, something went wrong.
|
@tacaswell Things look good, I should have all the backends (apart from the two mac ones) converted by the end of the weekend. I don't use macs, only linux and sparingly win. Does it matter if they don't get converted right now? Or I could try and code it blind. Also anything I should do to ease the review? @fariza suggested opening an MEP for this. |
Sorry, something went wrong.
|
Gah, help, why does this not build? I cannot see what I have changed since the last okay from travis that could have caused this build doc failiure... The build gives the following as one line. matplotlib/lib/matplotlib/backend_bases.py:docstring of matplotlib.backend_bases.WindowBase.destroy:2: WARNING: Inline emphasis start-string without end-string. |
Sorry, something went wrong.
|
I think you need to escape the * in that doc string TEXT is default for emphasising text in RST so it complains that you start an emphasised section but not end it. See http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#emphasis A backslash should do |
Sorry, something went wrong.
|
Thanks @jenshnielsen , that has fixed it :D. On a side note, when I run make.py html it gives me an intermittent error (every other build) Expected 2-dimensional array, got 1 on mplot3d/surface3d_demo.py Anything to do with #3675? |
Sorry, something went wrong.
|
Ok, now that is weird. surface3d_demo.py explicitly computes its own data: X = np.arange(-5, 5, 0.25) Y = np.arange(-5, 5, 0.25) X, Y = np.meshgrid(X, Y) R = np.sqrt(X**2 + Y**2) Z = np.sin(R) That is always 2d, no matter what. It would be helpful to know exactly On Tue, Mar 3, 2015 at 12:03 PM, OceanWolf notifications@github.com wrote:
|
Sorry, something went wrong.
|
I think I have seen that error when building the docs. It happened because I had a leftover old version of mpl_toolkits in my sitepackages folder from before everything was moved to the matplotlib folder. |
Sorry, something went wrong.
|
Define nights... the international nature of this project means we should discuss times in UTC (Universal Coordinated Time). |
Sorry, something went wrong.
|
In that case I'm UTC-4 So the best for me is after UTC 1:00am On Aug 9, 2016 8:15 AM, "OceanWolf" notifications@github.com wrote:
|
Sorry, something went wrong.
| self._is_gui = hasattr(self._backend, 'Window') | ||
| if not self._is_gui: | ||
| self.window = None | ||
| return |
There was a problem hiding this comment.
I don't think return here is going to work (init returns the object not a value/None).
What about moving the gui setup (window, toolbar, ....) to another method, that doesn't get called if is not gui.?
Sorry, something went wrong.
There was a problem hiding this comment.
give me copy paste code to show that it doesn't work... __init__ works as a normal method, i.e. it has an implicit return, i.e. return None at the end...
I think you get confused with __new__ as new does that... not sure what object.new looks like but you get the gist from this:
def __new__(class, *args, **kwargs):
instance = super().__new__(class, *args, **kwargs) # calls __init__(self, *args, **kwargs)
return instancebasically:
- From the code MyClass(*args, **kwargs)
2.__new__(MyClass, *args, **kwargs) gets called. - That magically creates the instance and calls __init__ on it.
- Returns the instance.
Sorry, something went wrong.
There was a problem hiding this comment.
I got confused because init must return None.
I just remembered that we shouldn't return things from init ;)
Sorry, something went wrong.
There was a problem hiding this comment.
yup, because it happens with __new__, you can do fun things with __new__...
class NaughtyBoy(object):
def __init__(self):
print('Just a naughty boy')
class Messiah(object):
def __init__(self):
print('Hallelujah!')
def __new__(cls, *args, **kwargs):
print('Not the Messiah')
return NaughtyBoy()
person = Messiah()
print('I wanted the Messiah, but I got a', type(person))
Sorry, something went wrong.
|
At the moment I work on UTC+2, so a bit difficult for us arrange a time... I guess I could work a night... |
Sorry, something went wrong.
|
Weekend before UTC 2pm? On Aug 15, 2016 6:37 PM, "OceanWolf" notifications@github.com wrote:
|
Sorry, something went wrong.
|
@tacaswell @mdboom @OceanWolf can we look for a time and date to take a look at this PR? I understand that there might be problems/corrections with the specific implementation, that is something we can work on. |
Sorry, something went wrong.
|
@tacaswell Can we include this in one of the weekly phone calls? |
Sorry, something went wrong.
|
A couple weeks ago we discussed this on the Monday conference and agreed that this work is the primary target for 2.1, so it has high priority as soon as 2.0 is out. |
Sorry, something went wrong.
|
Great! On Oct 22, 2016 4:17 PM, "Eric Firing" notifications@github.com wrote:
|
Sorry, something went wrong.
|
I have rebased this with the same name in my repo. |
Sorry, something went wrong.
|
Hello everyone
Can we plan sometime for reviewing?
Thanks
Federico
…On Feb 11, 2017 7:00 PM, "Thomas A Caswell" ***@***.***> wrote:
I have rebased this with the same name in my repo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4143 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABa86XPfk13LS9YaOROLLxsAuQMqgHHEks5rbkuugaJpZM4Dj1l2>
.
|
Sorry, something went wrong.
|
I will likely have time to look at this again in a couple of weeks.
On Wed, Apr 26, 2017 at 9:16 AM, Federico Ariza <notifications@github.com>
wrote:
… Hello everyone
Can we plan sometime for reviewing?
Thanks
Federico
On Feb 11, 2017 7:00 PM, "Thomas A Caswell" ***@***.***>
wrote:
> I have rebased this with the same name in my repo.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/matplotlib/matplotlib/pull/
4143#issuecomment-279185227>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/
ABa86XPfk13LS9YaOROLLxsAuQMqgHHEks5rbkuugaJpZM4Dj1l2>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4143 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-EKK4o_AfokYxk4SrG1HD_3J4ww5ks5rz0PAgaJpZM4Dj1l2>
.
|
Sorry, something went wrong.
|
If you think it could help.
We can do a couple of hangout sessions after you go through the code.
Federico
…On Apr 27, 2017 3:21 PM, "Benjamin Root" ***@***.***> wrote:
I will likely have time to look at this again in a couple of weeks.
On Wed, Apr 26, 2017 at 9:16 AM, Federico Ariza ***@***.***>
wrote:
> Hello everyone
>
> Can we plan sometime for reviewing?
>
> Thanks
> Federico
>
> On Feb 11, 2017 7:00 PM, "Thomas A Caswell" ***@***.***>
> wrote:
>
> > I have rebased this with the same name in my repo.
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <https://github.com/matplotlib/matplotlib/pull/
> 4143#issuecomment-279185227>,
> > or mute the thread
> > <https://github.com/notifications/unsubscribe-auth/
> ABa86XPfk13LS9YaOROLLxsAuQMqgHHEks5rbkuugaJpZM4Dj1l2>
>
> > .
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/matplotlib/matplotlib/pull/
4143#issuecomment-297404118>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AARy-EKK4o_
AfokYxk4SrG1HD_3J4ww5ks5rz0PAgaJpZM4Dj1l2>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4143 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABa86XQzvvBhBIhOTyViLmOQ5DdsPYuMks5r0OqzgaJpZM4Dj1l2>
.
|
Sorry, something went wrong.
|
Still out of it for a bit, hope to get back in to the groove soon. |
Sorry, something went wrong.
|
Instead of MainLoop.begin and MainLoop.end, I think it may be more pythonic to make MainLoop a contextmanager (__enter__ and __end__). |
Sorry, something went wrong.
Sorry, something went wrong.
This PR exists as an alternative to #2624
See https://github.com/OceanWolf/matplotlib/blob/backend-refactor/doc/devel/MEP/MEP27.rst for details.
Tl;dr:
FigureManagerBase ➡️ FigureManager + WindowBase
ShowBase ➡️ Gcf.show_all + MainLoopBase
The main refactor (+gtk3 as an example) goes here, and will have no effect on the individual backends, work for other backends will occur in separate branches as indicated.
To accomplish this, we shall do this in two passes: