Skip to content

Revert "Experimenting with import order" #478

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

Merged
merged 1 commit into from
Feb 3, 2014

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 3, 2014

See #474 and commit message.

This reverts commit c407a00, which selected the Agg backend for
Matplotlib in pymc/__init__.py, overriding the effects of 40a8070. These
changes were unnecessary to fix the non-interative display errors in the
Travis tests and prevent interactive plotting unless the user has
selected a backend prior to importing pymc.
@twiecki
Copy link
Member

twiecki commented Feb 3, 2014

What other fix do we have? Do we need one?

@kyleam
Copy link
Contributor Author

kyleam commented Feb 3, 2014

What other fix do we have? Do we need one?

Short answer: No other fix is needed. The default interactive backend is
used, but the non-interactive backend can still be selected by the
tests.

Longer:

Sorry, I must not be explaining this well in #474 and the commit
message.

The key problem with the display errors in Travis were caused because
pymc imports matplotlib.pyplot in the top-level init file. So when pymc
is imported, pyplot is as well, which selects the default (interactive)
backend. A matplotlib backend must be selected before pyplot is
imported, so the tests are no longer able to select the non-interactive
backend, leading to the display errors on Travis.

I submitted a commit (40a8070) that introduced function-level imports of
matplotlib.pyplot. This resolves the non-interactive display error
because now the test files can select Agg before any plot function is
called. If no backend is selected manually, the backend is the default
plotting backend (which will be interactive for most users). Everything
functions the same as usual, but there is the opportunity to select
another backend after pymc has been imported.

The above fix was merged into the "fix_travis" branch, but this has a
commit (c407a00) that unnecessarily selects the Agg backend in
pymc/init.py, making the changes in 40a8070 pointless because now
Agg is always selected. The only way the user could plot interactively
would be selecting the backend before pymc is imported.

Please let me know if this makes sense.

The tests still have some issues, but they are not the display issues.

680.{3,8}

'Could not import inplace_increment, so some advanced '
NotImplementedError: Could not import inplace_increment, so some
advanced indexing features are disabled. They will be available if
you update NumPy to version 1.8 or later, or to the latest
development version. You may need to clear the cache (theano-cache
clear) afterwards.

680.9

No output has been received in the last 10 minutes, this potentially
indicates a stalled build or something wrong with the build itself.

680.10

FloatingPointError: invalid value encountered in double_scalars

@twiecki
Copy link
Member

twiecki commented Feb 3, 2014

So the new fix is to simply move the matplotlib imports to the functions while before we kept it but changed the interface? Feels like a limitation to not be able to import matplotlib at the module level but since it's already in there this seems to improve the situation. Thanks!

twiecki added a commit that referenced this pull request Feb 3, 2014
Revert "Experimenting with import order"
@twiecki twiecki merged commit 693def7 into pymc-devs:master Feb 3, 2014
@kyleam
Copy link
Contributor Author

kyleam commented Feb 3, 2014

On 02/03/14 13:20, Thomas Wiecki wrote:

So the new fix is to simply move the matplotlib imports to the functions
while before we kept it but changed the interface?

This is the original fix that I proposed in #474 and that was merged
into the "fix_travis" branch. This current commit just reverts another
commit on the "fix_travis" branch that was unnecessary and that prevents
interactive plotting.

Feels like a limitation to not be able to import matplotlib at the
module level but since it's already in there this seems to improve the
situation

Sure, it is a restriction (though it follows these recommendations [1]
for how libraries should use matplotlib.pyplot). In comment #474,
@mwaskom describes how he solved this with a Travis-specific
matplotlibrc. Over there I mention why I prefer function-level pyplot
imports, but it would be nice to hear what others think. If the
Travis-specific configuration is preferred, it should take very little
effort to move to that.

[1] https://groups.google.com/forum/#!msg/pystatsmodels/biNlCvJPNNY/BT7bQJmOa1cJ

@kyleam kyleam deleted the remove-mpl-agg-init branch March 28, 2014 16:45
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.

2 participants