Skip to content

Fix xkcd() not resetting context anymore. #9603

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
Oct 28, 2017
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 28, 2017

Fixes #9520, replaces #9521.
Turns out ExitStack was not the easiest solution (as it gets (reasonably) implicitly exited upon GC).

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@@ -11,7 +11,7 @@
import pytest

import matplotlib as mpl
from matplotlib import style
from matplotlib import pyplot as plt, style
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, is this import style "ok"?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, doing import sys, os is what is discouraged.

In that case that you are importing mulitple top level things is obscured, in this case we are importing multiple things from the same namespace (and renaming one). https://www.python.org/dev/peps/pep-0008/#imports

@jklymak jklymak mentioned this pull request Oct 28, 2017
1 task
@Kojoley Kojoley merged commit 8989a6f into matplotlib:master Oct 28, 2017
@anntzer anntzer deleted the xkcd branch October 28, 2017 21:30
@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Mar 21, 2018

Is the intended consequence of this PR that

plt.xkcd()
plt.figure()
plt.plot(...)

is not working any more as in this example and that instead you have to use it as a context?

with plt.xkcd():
    plt.figure()
    plt.plot(...)

The latter is shown in this example.

Would it be possible to issue a warning if a function is used without a context?

@anntzer
Copy link
Contributor Author

anntzer commented Mar 21, 2018

No, that was not the intent and is a bug.
Sigh, circular references and garbage collection...

Basically, right now we have a test that the requested pattern works:

def test_xkcd_no_cm():
    assert mpl.rcParams["path.sketch"] is None
    plt.xkcd()
    assert mpl.rcParams["path.sketch"] == (1, 100, 2)

but that's only because the cyclic gc (usually) doesn't trigger just before the second assert; as:

from pylab import *
import gc
print(rcParams["path.sketch"])
plt.xkcd()
gc.collect()
print(rcParams["path.sketch"])
plt.show()

always gives None, None.

@phobson
Copy link
Member

phobson commented Mar 21, 2018

Bigger picture: Is the xkcd style/function/context manager really something that we want to continue supporting?

@anntzer
Copy link
Contributor Author

anntzer commented Mar 21, 2018

I don't have any problems with it sticking around, although switching to pythonic styles (per the "Python-syntax matplotlibrc" PR) would make this infinitely easier (would become a regular style).

@jklymak
Copy link
Member

jklymak commented Mar 21, 2018

I'd work on pythonic styles first, rather than trying to get this to work again.

@anntzer anntzer mentioned this pull request Mar 21, 2018
6 tasks
@ImportanceOfBeingErnest
Copy link
Member

As I see it the xkcd function shows the power of the library. It's kind of an advertisement of what is possible with matplotlib. So yes, I'm totally in favour of keeping that (although I don't care if it's a pyplot function or a style or usable only with contexts etc.).

Or maybe just because it's great fun. 😎

image

@afvincent
Copy link
Contributor

@anntzer Well, as you know (#6157), there is a challenge about converting plt.xkcd into a “regular” style by mean of an rcparams-file because it uses a patheffect rcparams, which cannot be parsed at the moment :/.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 21, 2018

We discussed the pythonic-mplrc proposal during this week's dev call (btw you should join, it's fun :-)) and looks like we'll make some progress on that.

@afvincent
Copy link
Contributor

Glad to hear that things are moving :) (did not read the notes from this week's call yet 🐑 )

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.

XKCD context manager not resetting anymore in 2.1
8 participants