Pylab fix #1052

Merged
merged 9 commits into from Nov 27, 2011

Projects

None yet

2 participants

@fperez
IPython member

When merging #648, we inadvertently broke pretty badly the pylab support. I added a test to catch the problem and started fixing things, but quickly saw we had a pretty messy setup in our pylab support machinery, just the product of the fast development we had during the creation of the qt console and zeromq support. The only way to fix this cleanly was to refactor things a little bit, but I think it's in decent shape now.

The main problem is that we had a ton of code duplication, with several methods implemented twice in the terminal and zmq shells, but with actually minimal differences. But the duplication meant that fixing a but required hunting down a lot of stuff around.

Now all duplicate code is gone, and the only real difference is how gui event loops are integrated, which is reduced to a single static method that each relevant class grabs from its specific machinery.

Merging this is, however, very urgent because right now master is fully broken. I'm going to see if @minrk is available on IRC and can have a quick look. If not, I'll merge it immediately, but having the pull request helps to easily see what I did in isolation, and we can do a post-merge reivew. I did verify that it fixes the problem we had, added new tests, and checked that both the qt console and the notebook work fine.

fperez added some commits Nov 27, 2011
@fperez fperez Fix critical bug with pylab support inadvertently introduced in #648.
code used it as a dict.  Updated that code to handle a dict correctly,
and added tests to catch this issue in the future (also increases test
coverage of pylab code).
cd84e0e
@fperez fperez Refactor gui/pylab integration to eliminate code duplication.
Also, fix a few tests that the previous commit broke.
cfd87e9
@fperez fperez Move zmq event loop support into a separate file.
This avoids a circular import problem, and also organizes more cleanly
the code that is event-loop specific.
0e21764
@fperez fperez Minor cleanups after a check with pyflakes of the refactored code. 5c4e9a0
@fperez fperez Add missing file. 37afbb3
@minrk minrk commented on an outdated diff Nov 27, 2011
IPython/core/pylabtools.py
+ shell : InteractiveShell instance
+ If None, this function returns immediately.
+
+ user_ns : dict
+ A namespace where all configured variables will be placed. If not given,
+ the `user_ns` attribute of the shell object is used.
+ """
+ if shell is None:
+ return
+
+ user_ns = shell.user_ns if user_ns is None else user_ns
+
+ # If using our svg payload backend, register the post-execution
+ # function that will pick up the results for display. This can only be
+ # done with access to the real shell object.
+ from IPython.zmq.pylab.backend_inline import InlineBackend
@minrk
minrk Nov 27, 2011

This will make ipython --pylab fail in the absence of zmq.

@minrk minrk commented on an outdated diff Nov 27, 2011
IPython/core/pylabtools.py
- exec s in shell.user_ns_hidden
+
+
+def configure_shell(shell, backend, user_ns=None):
+ """Configure an IPython shell object for matplotlib use.
+
+ Parameters
+ ----------
+ shell : InteractiveShell instance
+ If None, this function returns immediately.
+
+ user_ns : dict
+ A namespace where all configured variables will be placed. If not given,
+ the `user_ns` attribute of the shell object is used.
+ """
+ if shell is None:
@minrk
minrk Nov 27, 2011

I would just not call this function if shell is None - no need for it to conditionally do nothing if it was given inappropriate args.

@fperez
IPython member

Merging now after detailed review with @minrk on IRC. If anyone else spots any issues let me know, and I'm happy to revisit. But at least @minrk did have a careful look, and he spotted a number of problems (all fixed now).

@fperez fperez merged commit f15123a into ipython:master Nov 27, 2011
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@fperez fperez Avoid calling inline config if no shell - per @minrk feedback on #1052 c74d8de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment