Skip to content

fix xkcd context #9521

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

Closed
wants to merge 1 commit into from
Closed

fix xkcd context #9521

wants to merge 1 commit into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Oct 22, 2017

PR Summary

Fixes xkcd context so that one can do (as per #9520):

import matplotlib.pyplot as plt

with plt.xkcd():
    plt.figure()
    plt.plot([1, 2, 3])
    plt.savefig('xkcd.png', dpi=300)

plt.figure()
plt.plot([1, 2, 3])
plt.savefig('not_xkcd.png', dpi=300)

PR Checklist

  • Code is PEP 8 compliant

@tacaswell
Copy link
Member

I do not understand how either the change to the context manager nor this change fixes it.

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

Before it was just setting the rcParams to the new xkcd values and not passing them to the context. I don't understand how it worked before ;-)

@tacaswell
Copy link
Member

Ah, I understand. Before the clean up of rc_context the previous state was cached on __init__, now it is stashed on __enter__.

This PR fixes the case of a context manager, but I suspect breaks if you just call

plt.xkcd()
fig, ax = plt.subplots()
...

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

Um yeah I guess so.

I find this a strange function in that it passes parameters to a style.

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

I’d actually propose an xkcd style and deprecating this function. If folks want to monkey with the randomness of the lines they can call the style and then do so directly with the rcparams.

I’m too dense to know how to make this work as both a context and directly changing the rcParams.

@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

So you just need to add a call to context.__enter__() right after creating the context (in the old version of the code).
contextlib.ExitStack would make this easier to write :-)

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

Ok that’s easy.

@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

xkcd style will not work because the rcparams machinery is messed up (or pick a less strong qualifier) #6157

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

    from matplotlib import patheffects
    context = rc_context()
    context.__enter__()

yields

Traceback (most recent call last):
  File "testxkcd.py", line 3, in <module>
    with plt.xkcd():
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/contextlib.py", line 83, in __enter__
    raise RuntimeError("generator didn't yield") from None
RuntimeError: generator didn't yield

@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

Bah, we were doing something way too complex.

-    context = rc_context()
-    try:
-        rcParams['font.family'] = ['xkcd', 'Humor Sans', 'Comic Sans MS']
-        rcParams['font.size'] = 14.0
-        rcParams['path.sketch'] = (scale, length, randomness)
-        rcParams['path.effects'] = [
-            patheffects.withStroke(linewidth=4, foreground="w")]
-        rcParams['axes.linewidth'] = 1.5
-        rcParams['lines.linewidth'] = 2.0
-        rcParams['figure.facecolor'] = 'white'
-        rcParams['grid.linewidth'] = 0.0
-        rcParams['axes.grid'] = False
-        rcParams['axes.unicode_minus'] = False
-        rcParams['axes.edgecolor'] = 'black'
-        rcParams['xtick.major.size'] = 8
-        rcParams['xtick.major.width'] = 3
-        rcParams['ytick.major.size'] = 8
-        rcParams['ytick.major.width'] = 3
-    except:
-        context.__exit__(*sys.exc_info())
-        raise
-    return context
+    return rc_context({
+        'font.family': ['xkcd', 'Humor Sans', 'Comic Sans MS'],
+        'font.size': 14.0,
+        'path.sketch': (scale, length, randomness),
+        'path.effects': [patheffects.withStroke(linewidth=4, foreground="w")],
+        'axes.linewidth': 1.5,
+        'lines.linewidth': 2.0,
+        'figure.facecolor': 'white',
+        'grid.linewidth': 0.0,
+        'axes.grid': False,
+        'axes.unicode_minus': False,
+        'axes.edgecolor': 'black',
+        'xtick.major.size': 8,
+        'xtick.major.width': 3,
+        'ytick.major.size': 8,
+        'ytick.major.width': 3,
+    })

works (I tried it!).

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

I thought thats what I did ?

@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

... sorry, not paying attention. back to the drawing board.

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

Yeah, that doesn't work for @tacaswell case...

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

Sorry to be dense about the context management. I can close this PR and let you work on it if easier.

FWIW, @tacaswell case isn't actually documented or tested anywhere! Its implied in the docstring.

@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

this actually works in both cases

    from contextlib import ExitStack
    stack = ExitStack
    stack.enter_context(rc_context({
        'font.family': ['xkcd', 'Humor Sans', 'Comic Sans MS'], ...
    })
    return stack

(except that it needs a backport of contextlib.ExitStack (or similar) on AncientPython, e.g. https://pypi.python.org/pypi/contextlib2/0.5.5).

Basically we want to enter the rc_context but return another context that does nothing on enter but pops the rc_context on exit. ExitStack exactly provides that functionality.

As pointed out by @tacaswell #8962 indeed broke backcompat for whoever was using rc_context(...) without entering the context as a synonym for rc(...). I can't say I have much sympathy for this use case.

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

xkcd style will not work because the rcparams machinery is messed up (or pick a less strong qualifier)

Is this because path.effects wants a function, and there is no way to enter a function in a style sheet?

Is it possible to have a style-sheet parser fcn that reads a string from path.effects and makes sure it is calling something from patheffects before setting it?

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

OK, I suggest this change is consistent w/ the most documentation for now.

I suggest a way forward is to parse style sheets for strings that have patheffect calls, but to explicitly call patheffect.XX ourselves to prevent security risks. There may be a fancier way to do that. But then folks could just styles.use('xkcd') like any other style. See #6157

@jklymak
Copy link
Member Author

jklymak commented Oct 28, 2017

Closing in lieu of #9603

@jklymak jklymak closed this Oct 28, 2017
@tacaswell
Copy link
Member

Thanks for your work on this @jklymak !

@jklymak
Copy link
Member Author

jklymak commented Oct 28, 2017

No problem - @anntzer solution is very cool!

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.

3 participants