Add a "sketch" path filter #1329

Merged
merged 13 commits into from May 14, 2013

Conversation

Projects
None yet
@mdboom
Owner

mdboom commented Oct 5, 2012

Based on the mailing list thread "XKCD style graphs", this adds a sketchy line path filter to matplotlib.

This PR is not complete, but I thought I'd get something up early so we can hack on it together.

To play with this, set the rcParam['path.sketch'] to a tuple (scale, length, randomness), for example (1.0, 128., 16.).

          * *scale*: The amplitude of the wiggle perpendicular to the
            source line, in pixels.

          * *length*: The length of the wiggle along the line, in
             pixels (default 128.0)

          * *randomness*: The scale factor by which the length is
            shrunken or expanded (default 16.0)

This currently only works with the Agg and PDF backends, though it should be easy enough to add to other vector backends by following the same procedure as the PDF backend. The Mac OS-X backend will require more care, but should be possible as it's been added to path_cleanup.cpp.

Beyond this, it would be nice to add a convenience function xkcd_style, or some such, that thickens the lines, removes the north and east spines, possibly changes the font, etc.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Oct 5, 2012

Owner

Also, I invented my own wiggliness algorithm here based on a randomly progressing sine wave. I think Juergen Hasch's filtered noise approach looks better -- we should probably change this to do that.

Owner

mdboom commented Oct 5, 2012

Also, I invented my own wiggliness algorithm here based on a randomly progressing sine wave. I think Juergen Hasch's filtered noise approach looks better -- we should probably change this to do that.

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Oct 5, 2012

Contributor

Haha, looks great :)
mpl-xkcd

Contributor

pwuertz commented Oct 5, 2012

Haha, looks great :)
mpl-xkcd

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Oct 5, 2012

Owner

That image seems to be a broken link, maybe? No worries -- just curious what you've decided to do with it.

Owner

mdboom commented Oct 5, 2012

That image seems to be a broken link, maybe? No worries -- just curious what you've decided to do with it.

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Oct 5, 2012

Contributor

Oops, my bad. I better put it on github...

Contributor

pwuertz commented Oct 5, 2012

Oops, my bad. I better put it on github...

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Oct 5, 2012

Member

Let's see Matlab match this! :-P

Member

WeatherGod commented Oct 5, 2012

Let's see Matlab match this! :-P

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Oct 5, 2012

Owner

Very cool.

Owner

mdboom commented Oct 5, 2012

Very cool.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 5, 2012

Member

Amazing. Unfortunately, this won't make it in 1.2; it's not a bug fix.

:)

Member

dmcdougall commented Oct 5, 2012

Amazing. Unfortunately, this won't make it in 1.2; it's not a bug fix.

:)

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 5, 2012

Member

This is actually great for doing schematic 'big-picture' plots in a talk where a clean looking plot looks 'too scientific'.

Member

dmcdougall commented Oct 5, 2012

This is actually great for doing schematic 'big-picture' plots in a talk where a clean looking plot looks 'too scientific'.

lib/matplotlib/rcsetup.py
@@ -337,7 +337,16 @@ def validate_bbox(s):
return None
raise ValueError("bbox should be 'tight' or 'standard'")
-
+def validate_sketch(s):
+ if s == 'None' or s is None:

This comment has been minimized.

Show comment Hide comment
@NelleV

NelleV Oct 5, 2012

Contributor

s isn't a very explicit variable name. I'm not sure what the matplotlib's coding convention are regarding to this, but I think the code would gain in readibility with a longer variable name such as sketch.

@NelleV

NelleV Oct 5, 2012

Contributor

s isn't a very explicit variable name. I'm not sure what the matplotlib's coding convention are regarding to this, but I think the code would gain in readibility with a longer variable name such as sketch.

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Oct 5, 2012

Owner

In cases like this, however, I think using a short variable name is fine. "def validate_sketch(sketch)" looks worse to my eye.

@efiring

efiring Oct 5, 2012

Owner

In cases like this, however, I think using a short variable name is fine. "def validate_sketch(sketch)" looks worse to my eye.

This comment has been minimized.

Show comment Hide comment
@NelleV

NelleV Oct 9, 2012

Contributor

Dunno... Reading the code as a "not matplotlib expert", it didn't seem obvious what s was. I have for rule not to use one letter variables as they are often confusing for beginners, and I want my code to be easily read and modified by anyone. Explicit is better than implicit.

@NelleV

NelleV Oct 9, 2012

Contributor

Dunno... Reading the code as a "not matplotlib expert", it didn't seem obvious what s was. I have for rule not to use one letter variables as they are often confusing for beginners, and I want my code to be easily read and modified by anyone. Explicit is better than implicit.

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Oct 9, 2012

Owner

Your suggestion for a name, "sketch" would not help--it is no more descriptive of what "s" is than "s". "s" is not a sketch, it is an argument of unknown type being checked to see if it takes one of several valid forms.

@efiring

efiring Oct 9, 2012

Owner

Your suggestion for a name, "sketch" would not help--it is no more descriptive of what "s" is than "s". "s" is not a sketch, it is an argument of unknown type being checked to see if it takes one of several valid forms.

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 9, 2012

Member

@NelleV @efiring I understand you both have your opinions. They are also both valid opinions. That being said, it seems as though the best course of action here is to agree to disagree.

@dmcdougall

dmcdougall Oct 9, 2012

Member

@NelleV @efiring I understand you both have your opinions. They are also both valid opinions. That being said, it seems as though the best course of action here is to agree to disagree.

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Oct 6, 2012

Member

Probably should make it clear in any documentation that this parameter has to be set prior to any plotting calls, much like with setting font sizes and such.

Member

WeatherGod commented Oct 6, 2012

Probably should make it clear in any documentation that this parameter has to be set prior to any plotting calls, much like with setting font sizes and such.

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Oct 6, 2012

Member

sweet! add the plt.rc('path', sketch=(1.0, 128.0, 16.0)) towards the beginning of the animate_decay.py example for some fun!

http://youtu.be/PRRRvIRWk9M

Member

WeatherGod commented Oct 6, 2012

sweet! add the plt.rc('path', sketch=(1.0, 128.0, 16.0)) towards the beginning of the animate_decay.py example for some fun!

http://youtu.be/PRRRvIRWk9M

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Oct 6, 2012

Member

And doing it to the subplots.py example is really neat!

http://youtu.be/jx4D2NGqkd0

And it also works for 3D plots as well:
Lorenz Sketch

Member

WeatherGod commented Oct 6, 2012

And doing it to the subplots.py example is really neat!

http://youtu.be/jx4D2NGqkd0

And it also works for 3D plots as well:
Lorenz Sketch

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Oct 6, 2012

I don't have time to test this now, but I just want to say that I like this a lot!

certik commented Oct 6, 2012

I don't have time to test this now, but I just want to say that I like this a lot!

@glyg

This comment has been minimized.

Show comment Hide comment
@glyg

glyg Oct 6, 2012

Contributor

That's really nice! Now fetch the Ipython crew so we have a whole xkcd python notebook

Contributor

glyg commented Oct 6, 2012

That's really nice! Now fetch the Ipython crew so we have a whole xkcd python notebook

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Oct 6, 2012

Member

Ok, some bugs:

  1. The GraphicsContextBase doesn't copy the ._sketch attribute when creating a copy of itself.
  2. Artist.update_from() doesn't copy the ._sketch attribute (as well as some others...) (side note, some of the setters in Artist should probably be calling pchanged()).
  3. Even after these changes, I still can not seem to get various collection objects to sketch, so somewhere in chain, the sketch params are not getting propagated.
Member

WeatherGod commented Oct 6, 2012

Ok, some bugs:

  1. The GraphicsContextBase doesn't copy the ._sketch attribute when creating a copy of itself.
  2. Artist.update_from() doesn't copy the ._sketch attribute (as well as some others...) (side note, some of the setters in Artist should probably be calling pchanged()).
  3. Even after these changes, I still can not seem to get various collection objects to sketch, so somewhere in chain, the sketch params are not getting propagated.
@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Oct 6, 2012

Contributor

@glyg: A notebook for you: view | download

(Not my doing - I just saw a link to it)

Contributor

takluyver commented Oct 6, 2012

@glyg: A notebook for you: view | download

(Not my doing - I just saw a link to it)

@jakevdp

This comment has been minimized.

Show comment Hide comment
@jakevdp

jakevdp Oct 6, 2012

Contributor

Another notebook with some stabs in this direction (though done at the patch level in matplotlib):
http://nbviewer.ipython.org/url/jakevdp.github.com/downloads/notebooks/XKCD_plots.ipynb
Would it be possible for the sketch filter to automatically add a white background line so that there's the bit of space between line crossings?

Contributor

jakevdp commented Oct 6, 2012

Another notebook with some stabs in this direction (though done at the patch level in matplotlib):
http://nbviewer.ipython.org/url/jakevdp.github.com/downloads/notebooks/XKCD_plots.ipynb
Would it be possible for the sketch filter to automatically add a white background line so that there's the bit of space between line crossings?

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Oct 7, 2012

Member

On Saturday, October 6, 2012, Jacob Vanderplas wrote:

Another notebook with some stabs in this direction (though done at the
patch level in matplotlib):

http://nbviewer.ipython.org/url/jakevdp.github.com/downloads/notebooks/XKCD_plots.ipynb
Would it be possible for the sketch filter to automatically add a white
background line so that there's the bit of space between line crossings?

No, that is going to have to be done as a path effect or as an offset
transform.

Member

WeatherGod commented Oct 7, 2012

On Saturday, October 6, 2012, Jacob Vanderplas wrote:

Another notebook with some stabs in this direction (though done at the
patch level in matplotlib):

http://nbviewer.ipython.org/url/jakevdp.github.com/downloads/notebooks/XKCD_plots.ipynb
Would it be possible for the sketch filter to automatically add a white
background line so that there's the bit of space between line crossings?

No, that is going to have to be done as a path effect or as an offset
transform.

@Nikratio

This comment has been minimized.

Show comment Hide comment
@Nikratio

Nikratio Nov 18, 2012

@WeatherGod: In your animation examples, even lines that are not being changed (e.g. x and y axis) are constantly wobbling around. Did you do that deliberately, or is that an unintended side effect? To me it looks very irritating.

@WeatherGod: In your animation examples, even lines that are not being changed (e.g. x and y axis) are constantly wobbling around. Did you do that deliberately, or is that an unintended side effect? To me it looks very irritating.

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Nov 18, 2012

Member

On Saturday, November 17, 2012, Nikolaus Rath wrote:

@WeatherGod https://github.com/WeatherGod: In your animation examples,
even lines that are not being changed (e.g. x and y axis) are constantly
wobbling around. Did you do that deliberately, or is that an unintended
side effect? To me it looks very irritating.

The filter is applied whenever the coordinates of the path is accessed. It
is a natural consequence of the implementation. I rather like it. It
reminds me of some cartoons I remember. We would have to figure out a way
to make the randomness be applied once (maybe on-demand?).

Member

WeatherGod commented Nov 18, 2012

On Saturday, November 17, 2012, Nikolaus Rath wrote:

@WeatherGod https://github.com/WeatherGod: In your animation examples,
even lines that are not being changed (e.g. x and y axis) are constantly
wobbling around. Did you do that deliberately, or is that an unintended
side effect? To me it looks very irritating.

The filter is applied whenever the coordinates of the path is accessed. It
is a natural consequence of the implementation. I rather like it. It
reminds me of some cartoons I remember. We would have to figure out a way
to make the randomness be applied once (maybe on-demand?).

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Mar 4, 2013

Contributor

Any reason not to merge this?

Contributor

Tillsten commented Mar 4, 2013

Any reason not to merge this?

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 5, 2013

Owner

The api for using this is horrible. It needs to have proper kwarg or rcparam access before its mergeable. I've sort of dropped the ball on this one.

Owner

mdboom commented Mar 5, 2013

The api for using this is horrible. It needs to have proper kwarg or rcparam access before its mergeable. I've sort of dropped the ball on this one.

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Mar 5, 2013

Member

Also note the bugs that I have pointed out earlier. I think this was a great and innovative proof-of-concept, but it is probably best to re-work this.

A thought I did have about a possible way to address @Nikratio 's concerns (and would enable proper unit-testing) is an ability to cache a seed number for a path. That way, the random seed would be re-used at each re-draw, resulting in the same sketch every time. This seed could be automatically determined and/or explicitly set.

Member

WeatherGod commented Mar 5, 2013

Also note the bugs that I have pointed out earlier. I think this was a great and innovative proof-of-concept, but it is probably best to re-work this.

A thought I did have about a possible way to address @Nikratio 's concerns (and would enable proper unit-testing) is an ability to cache a seed number for a path. That way, the random seed would be re-used at each re-draw, resulting in the same sketch every time. This seed could be automatically determined and/or explicitly set.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Mar 6, 2013

Member

FWIW, if my memory serves me, I have a pretty good implementation of this which uses transforms only (with just a couple of bug fixes and if I remember correctly, a single monkey patch). The path filter stuff sounds very useful in this case, but it also sounds like a lot of work and I wonder how widely used it would really be?

Member

pelson commented Mar 6, 2013

FWIW, if my memory serves me, I have a pretty good implementation of this which uses transforms only (with just a couple of bug fixes and if I remember correctly, a single monkey patch). The path filter stuff sounds very useful in this case, but it also sounds like a lot of work and I wonder how widely used it would really be?

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Mar 6, 2013

I just wanted to say that it would be very nice to have this in matplotlib,
I think that a lot of people were waiting for this to get in. But I
understand of course that if the code is hackish, it's not worth the pain.

Ondrej

On Wed, Mar 6, 2013 at 12:34 PM, Phil Elson notifications@github.comwrote:

FWIW I have a pretty good implementation of this which uses transforms
only (with just a couple of bug fixes and if I remember correctly, a single
monkey patch). The path filter stuff sounds very useful in this case, but
it also sounds like a lot of work and I wonder how widely used it would
really be?


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/1329#issuecomment-14494827
.

certik commented Mar 6, 2013

I just wanted to say that it would be very nice to have this in matplotlib,
I think that a lot of people were waiting for this to get in. But I
understand of course that if the code is hackish, it's not worth the pain.

Ondrej

On Wed, Mar 6, 2013 at 12:34 PM, Phil Elson notifications@github.comwrote:

FWIW I have a pretty good implementation of this which uses transforms
only (with just a couple of bug fixes and if I remember correctly, a single
monkey patch). The path filter stuff sounds very useful in this case, but
it also sounds like a lot of work and I wonder how widely used it would
really be?


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/1329#issuecomment-14494827
.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Mar 6, 2013

Member

Thanks for the info Ondrej.

I think that a lot of people were waiting for this to get in.

Is it purely for XKCD style plots which show data in a less clinical, semantic form, or are there other usecases which make use of the path filter functionality?

Member

pelson commented Mar 6, 2013

Thanks for the info Ondrej.

I think that a lot of people were waiting for this to get in.

Is it purely for XKCD style plots which show data in a less clinical, semantic form, or are there other usecases which make use of the path filter functionality?

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 13, 2013

Owner

Trying to revive this PR in time for 1.3.0. I've added a top-level function xkcd() which tweaks a bunch of rcParams to make this work more-or-less automatically in most cases. I've run this over all of the gallery examples and it does a reasonable job in most cases.

It would be nice to ship the "Humor Sans" font, but I can't figure out what the license on it is. Anyone know?

And for your amusement: http://matplotlib.org/xkcd/ 😉

Owner

mdboom commented May 13, 2013

Trying to revive this PR in time for 1.3.0. I've added a top-level function xkcd() which tweaks a bunch of rcParams to make this work more-or-less automatically in most cases. I've run this over all of the gallery examples and it does a reasonable job in most cases.

It would be nice to ship the "Humor Sans" font, but I can't figure out what the license on it is. Anyone know?

And for your amusement: http://matplotlib.org/xkcd/ 😉

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 13, 2013

Owner

I should add: this PR now incorporates #1909.

Owner

mdboom commented May 13, 2013

I should add: this PR now incorporates #1909.

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik May 13, 2013

This is awesome!

certik commented May 13, 2013

This is awesome!

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 13, 2013

Owner

@pelson: to address your question about the path filter: I implemented it this way because if one were to use a transform, it would also have to interpolate along the straight line segments in order to apply the random walk. Since one wants the interpolation to happen at the scale of one pixel, regardless of the data transformation, it's rather hard to do that in numpy, since the size of the input and outputs are not the same and hard to estimate up front.

Owner

mdboom commented May 13, 2013

@pelson: to address your question about the path filter: I implemented it this way because if one were to use a transform, it would also have to interpolate along the straight line segments in order to apply the random walk. Since one wants the interpolation to happen at the scale of one pixel, regardless of the data transformation, it's rather hard to do that in numpy, since the size of the input and outputs are not the same and hard to estimate up front.

doc/users/whats_new.rst
+
+To gives your plots a sense of authority that they may be missing,
+Michael Droettboom (inspired by the work of many others in `issue
+#1329 <https://github.com/matplotlib/matplotlib/pull/1329>`_) has

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

We have a role to make the linking easier (ghissue I think) - see doc/sphinxext/github.py

@pelson

pelson May 14, 2013

Member

We have a role to make the linking easier (ghissue I think) - see doc/sphinxext/github.py

examples/showcase/xkcd.py
@@ -0,0 +1,40 @@
+from matplotlib import pyplot as plt

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

It'd be nice to reference the XKCD chart that is being reproduced.

@pelson

pelson May 14, 2013

Member

It'd be nice to reference the XKCD chart that is being reproduced.

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Picky: its fewer characters (and more consistent?) to type import matplotlib.pyplot as plt

@pelson

pelson May 14, 2013

Member

Picky: its fewer characters (and more consistent?) to type import matplotlib.pyplot as plt

doc/users/whats_new.rst
+To gives your plots a sense of authority that they may be missing,
+Michael Droettboom (inspired by the work of many others in `issue
+#1329 <https://github.com/matplotlib/matplotlib/pull/1329>`_) has
+added an `xkcd-style <xkcd.com>`_ sketch plotting mode. To use it,

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

This link is local - so results in a 404.

@pelson

pelson May 14, 2013

Member

This link is local - so results in a 404.

@@ -21,6 +21,18 @@ revision, see the :ref:`github-stats`.
new in matplotlib-1.3

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Path effects on lines deserves to go in here too.

@pelson

pelson May 14, 2013

Member

Path effects on lines deserves to go in here too.

+ plt.setp(cntr.collections,
+ path_effects=[PathEffects.withStroke(linewidth=3,
+ foreground="w")])
+

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Picky: too many newlines

@pelson

pelson May 14, 2013

Member

Picky: too many newlines

examples/pylab_examples/to_numeric.py
@@ -30,5 +30,4 @@
X.shape = h, w, 3
im = Image.fromstring( "RGB", (w,h), s)
-im.show()
-
+# im.show()

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Intentional?

@pelson

pelson May 14, 2013

Member

Intentional?

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

Yes -- though I should probably remove the line altogether -- this causes ImageMagick's "display" utility to be called during the documentation build.

@mdboom

mdboom May 14, 2013

Owner

Yes -- though I should probably remove the line altogether -- this causes ImageMagick's "display" utility to be called during the documentation build.

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Oh! That annoys me everytime and I've never got to the bottom of it! 😄

@pelson

pelson May 14, 2013

Member

Oh! That annoys me everytime and I've never got to the bottom of it! 😄

@@ -456,6 +458,52 @@ def set_snap(self, snap):
"""
self._snap = snap
+ def get_sketch_params(self):

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

I'd be keen to use numpydoc form here (https://github.com/matplotlib/matplotlib/wiki/Mep10)

@pelson

pelson May 14, 2013

Member

I'd be keen to use numpydoc form here (https://github.com/matplotlib/matplotlib/wiki/Mep10)

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

Ah, sure -- I didn't notice that. This PR predates MEP10, so I forgot to update it.

@mdboom

mdboom May 14, 2013

Owner

Ah, sure -- I didn't notice that. This PR predates MEP10, so I forgot to update it.

@@ -1003,6 +1005,42 @@ def get_hatch_path(self, density=6.0):
return None
return Path.hatch(self._hatch, density)
+ def get_sketch_params(self):
+ """

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Not an action: We should think about removing the duplicated docstings (duplicated in Artist) from here.

@pelson

pelson May 14, 2013

Member

Not an action: We should think about removing the duplicated docstings (duplicated in Artist) from here.

+ """
+ return self._sketch
+
+ def set_sketch_params(self, scale=None, length=None, randomness=None):

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Technically I don't think these need to be keywords as they are always given (in the draw method). But I agree that keeping the signature consistent is useful.

@pelson

pelson May 14, 2013

Member

Technically I don't think these need to be keywords as they are always given (in the draw method). But I agree that keeping the signature consistent is useful.

lib/matplotlib/collections.py
+ gc.set_sketch_params(*self.get_sketch_params())
+
+ if self.get_path_effects():
+ #from patheffects import PathEffectsRenderer

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Rogue comment.

@pelson

pelson May 14, 2013

Member

Rogue comment.

+ self._linewidths, self._linestyles, self._antialiaseds, self._urls,
+ self._offset_position)
+ else:
+

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Unnecessary newline.

@pelson

pelson May 14, 2013

Member

Unnecessary newline.

lib/matplotlib/patheffects.py
@@ -22,6 +21,9 @@ def __init__(self):
"""
super(_Base, self).__init__()
+ def get_proxy_renderer(self, renderer):
+ pe_renderer = ProxyRenderer(self, renderer)

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Just return it?

@pelson

pelson May 14, 2013

Member

Just return it?

+ path, transform = path_id
+ transform = transforms.Affine2D(transform.get_matrix()).translate(xo, yo)
+ self.draw_path(renderer, gc0, path, transform, rgbFace)
+

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Rogue newline.

@pelson

pelson May 14, 2013

Member

Rogue newline.

@@ -51,6 +53,50 @@ def draw_path(self, renderer, gc, tpath, affine, rgbFace):
"""
renderer.draw_path(gc, tpath, affine, rgbFace)
+ def draw_path_collection(self, renderer,

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

As far as I can work out, this is a copy and paste of RendererBase's method. Does this suggest that _Base should be subclassing RendererBase...

@leejjoon - thoughts?

@pelson

pelson May 14, 2013

Member

As far as I can work out, this is a copy and paste of RendererBase's method. Does this suggest that _Base should be subclassing RendererBase...

@leejjoon - thoughts?

This comment has been minimized.

Show comment Hide comment
@leejjoon

leejjoon May 14, 2013

Contributor

Yes, this is a copy of RendererBases method. Well, _Base implements only a subset of RendererBase's methods, and I am not sure if it is a good idea to subclass RendereBase. We may consider some more refactoring though.

@leejjoon

leejjoon May 14, 2013

Contributor

Yes, this is a copy of RendererBases method. Well, _Base implements only a subset of RendererBase's methods, and I am not sure if it is a good idea to subclass RendereBase. We may consider some more refactoring though.

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

@pelson -- inheriting from RendererBase might make some sense, but we can probably put that off for a later PR.

@mdboom

mdboom May 14, 2013

Owner

@pelson -- inheriting from RendererBase might make some sense, but we can probably put that off for a later PR.

+ rgbFace)
+
+
+class ProxyRenderer(object):

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

A docstring? Why does this exist?

@pelson

pelson May 14, 2013

Member

A docstring? Why does this exist?

lib/matplotlib/pyplot.py
@@ -255,6 +255,38 @@ def setp(*args, **kwargs):
draw_if_interactive()
return ret
+def xkcd():
+ """
+ Turns on `xkcd <xkcd.com>`_ sketch-style drawing mode. This will

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Link is a 404.

@pelson

pelson May 14, 2013

Member

Link is a 404.

lib/matplotlib/pyplot.py
+ Turns on `xkcd <xkcd.com>`_ sketch-style drawing mode. This will
+ only have effect on things drawn after this function is called.
+
+ For best results, the "Humor Sans" font should be installed: it is

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

How? Is there a link?

@pelson

pelson May 14, 2013

Member

How? Is there a link?

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

It's hard to tell what the canonical URL for this font should be.

@mdboom

mdboom May 14, 2013

Owner

It's hard to tell what the canonical URL for this font should be.

lib/matplotlib/pyplot.py
@@ -255,6 +255,38 @@ def setp(*args, **kwargs):
draw_if_interactive()
return ret
+def xkcd():

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

I'm not the biggest fan of globals - how about we make xkcd a context manager?

with plt.xkcd():
    plt.plot(range(10))
    plt.show()
@pelson

pelson May 14, 2013

Member

I'm not the biggest fan of globals - how about we make xkcd a context manager?

with plt.xkcd():
    plt.plot(range(10))
    plt.show()

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

It's probably not a bad idea to optionally make it a context manager, but I don't think it should be required. The imposes a level of structure that's beyond the quick-and-dirty plotting scripts that most people write.

@mdboom

mdboom May 14, 2013

Owner

It's probably not a bad idea to optionally make it a context manager, but I don't think it should be required. The imposes a level of structure that's beyond the quick-and-dirty plotting scripts that most people write.

@@ -740,9 +754,10 @@ def __call__(self, s):
'path.simplify': [True, validate_bool],
'path.simplify_threshold': [1.0 / 9.0, ValidateInterval(0.0, 1.0)],
'path.snap': [True, validate_bool],
+ 'path.sketch': [None, validate_sketch],
+ 'path.effects': [[], validate_any],

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

I think this goes against most other rcParams - I have no idea what is a valid path.effect value...

@pelson

pelson May 14, 2013

Member

I think this goes against most other rcParams - I have no idea what is a valid path.effect value...

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

It's not really meant to be public -- just needed a place to store global state that would be resettable. Perhaps rename to _path.effects to make its private nature clearer?

@mdboom

mdboom May 14, 2013

Owner

It's not really meant to be public -- just needed a place to store global state that would be resettable. Perhaps rename to _path.effects to make its private nature clearer?

@@ -0,0 +1,54 @@
+import numpy as np

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

👍 for adding a test. Needs to be added to lib/matplotlib/__init__.py though.

@pelson

pelson May 14, 2013

Member

👍 for adding a test. Needs to be added to lib/matplotlib/__init__.py though.

+ double sketch_randomness = 0.0;
+ if (sketch_params.ptr() != Py_None) {
+ Py::Tuple sketch(sketch_params);
+ sketch_scale = Py::Float(sketch[0]);

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

Nope -- this is the PyCXX interface that will raise a C++ exception if it fails.

@mdboom

mdboom May 14, 2013

Owner

Nope -- this is the PyCXX interface that will raise a C++ exception if it fails.

@@ -3,12 +3,14 @@
#ifndef __PATH_CONVERTERS_H__

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Technically shouldn't this file be called src/path_converters.cpp?

@pelson

pelson May 14, 2013

Member

Technically shouldn't this file be called src/path_converters.cpp?

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

Long story short -- templatized classes need to be implemented in a header so that the compiler can instantiate the templates. Once they are in object files (which a .cpp file would compile into) the template is static and can not be filled in by the compiler. You can see how it gets used from _backend_agg.cpp and path_cleanup.cpp for example.

@mdboom

mdboom May 14, 2013

Owner

Long story short -- templatized classes need to be implemented in a header so that the compiler can instantiate the templates. Once they are in object files (which a .cpp file would compile into) the template is static and can not be filled in by the compiler. You can see how it gets used from _backend_agg.cpp and path_cleanup.cpp for example.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Mike: This is looking good. Please only add commits on top of c288df2 (rather than rebasing / modifying any history) and I would be happy to merge after some of the actions have been addressed/discussed.

I love the xkcd rendered version of the docs - very nicely done 😄

Member

pelson commented May 14, 2013

Mike: This is looking good. Please only add commits on top of c288df2 (rather than rebasing / modifying any history) and I would be happy to merge after some of the actions have been addressed/discussed.

I love the xkcd rendered version of the docs - very nicely done 😄

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

@pelson: Thanks for the feedback. I believe I have addressed all of your comments. The context manager took some work because the font.* rcParams are "dynamic". I've changed this so that they only affect to-be-created text objects and not ones already created (this brings them in line with most other rcParams). That's a pretty significant change, but it surprisingly doesn't break any tests. Still something to take some caution about.

Out of curiosity, why the rebase warning?

Owner

mdboom commented May 14, 2013

@pelson: Thanks for the feedback. I believe I have addressed all of your comments. The context manager took some work because the font.* rcParams are "dynamic". I've changed this so that they only affect to-be-created text objects and not ones already created (this brings them in line with most other rcParams). That's a pretty significant change, but it surprisingly doesn't break any tests. Still something to take some caution about.

Out of curiosity, why the rebase warning?

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Out of curiosity, why the rebase warning?

I didn't want to re-read the whole lot again 😄.

This all looks good to me. Very nice work 😄

Member

pelson commented May 14, 2013

Out of curiosity, why the rebase warning?

I didn't want to re-read the whole lot again 😄.

This all looks good to me. Very nice work 😄

pelson added a commit that referenced this pull request May 14, 2013

Merge pull request #1329 from mdboom/wiggles
Add a "sketch" path filter for XKCD style graphics.

@pelson pelson merged commit 6b442b8 into matplotlib:master May 14, 2013

1 check was pending

default The Travis CI build is in progress
Details
@@ -58,7 +58,7 @@ class _path_module : public Py::ExtensionModule<_path_module>
add_varargs_method("convert_path_to_polygons", &_path_module::convert_path_to_polygons,
"convert_path_to_polygons(path, trans, width, height)");
add_varargs_method("cleanup_path", &_path_module::cleanup_path,
- "cleanup_path(path, trans, remove_nans, clip, snap, simplify, curves)");
+ "cleanup_path(path, trans, remove_nans, clip, snap, simplify, curves, sketch_params)");

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 15, 2013

Member

@mdboom - I don't this this string is correct.

@pelson

pelson May 15, 2013

Member

@mdboom - I don't this this string is correct.

}
}
Py::Object
_path_module::cleanup_path(const Py::Tuple& args)
{
- args.verify_length(8);
+ args.verify_length(9);

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 15, 2013

Member

This is a backwards incompatible change to a public module function. Would you mind adding this to the doc/api/api_changes.rst?

@pelson

pelson May 15, 2013

Member

This is a backwards incompatible change to a public module function. Would you mind adding this to the doc/api/api_changes.rst?

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 15, 2013

Member

Actually, I've got a change incoming in which I will add the documentation.

@pelson

pelson May 15, 2013

Member

Actually, I've got a change incoming in which I will add the documentation.

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 15, 2013

Owner

_path is a private module -- it should only be accessed publicly through path.py, so I don't think we need to consider this a public API change.

@mdboom

mdboom May 15, 2013

Owner

_path is a private module -- it should only be accessed publicly through path.py, so I don't think we need to consider this a public API change.

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 15, 2013

Member

Whilst I agree that it isn't defined in path, it is certainly part of path's namespace:

>>> import matplotlib.path
>>> print matplotlib.path.cleanup_path.__doc__
cleanup_path(path, trans, remove_nans, clip, snap, simplify, curves, sketch_params)
@pelson

pelson May 15, 2013

Member

Whilst I agree that it isn't defined in path, it is certainly part of path's namespace:

>>> import matplotlib.path
>>> print matplotlib.path.cleanup_path.__doc__
cleanup_path(path, trans, remove_nans, clip, snap, simplify, curves, sketch_params)

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 15, 2013

Owner

Ah -- good point. That's unintentional... Path.iter_segments provides a much more Pythonic interface on top of the same functionality. I guess we should deprecate these C++ functions now and hide them completely in 1.4. I'll file a new issue for this.

@mdboom

mdboom May 15, 2013

Owner

Ah -- good point. That's unintentional... Path.iter_segments provides a much more Pythonic interface on top of the same functionality. I guess we should deprecate these C++ functions now and hide them completely in 1.4. I'll file a new issue for this.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 15, 2013

Member

Was this supposed to be 'font.stretch'? Relates to #2006

Was this supposed to be 'font.stretch'? Relates to #2006

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 15, 2013

Owner

Indeed. Sorry about that -- didn't take long to find, though. Fix in de956ef

Owner

mdboom replied May 15, 2013

Indeed. Sorry about that -- didn't take long to find, though. Fix in de956ef

@leejjoon

This comment has been minimized.

Show comment Hide comment
@leejjoon

leejjoon May 15, 2013

Contributor

@mdboom : is this change necessary? This causes an error with texmanager.py, which expects rcParams["font.family"] to be a string. And I guess there are user codes that will fail with this change.

@mdboom : is this change necessary? This causes an error with texmanager.py, which expects rcParams["font.family"] to be a string. And I guess there are user codes that will fail with this change.

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 15, 2013

Owner

Ah -- I missed that about texmanager. The other text handling handled this just fine without further changes. I think maybe we should try to fix texmanager to support a list (see #2012).

Here the use case that brought this about:

The xkcd function needs to change the font of all text that is created while it is in effect, and then go back to the previous settings afterward. If you set font.family to fantasy and change font.fantasy to Humor Sans, this doesn't work, because the lookup from a category name ('fantasy') to a concrete name is dynamic so when font.family is reverted, so is all of the text that was created in the xkcd context. I think this is the correct behavior for category names, as they are supposed to be dynamically updatable. So the only way to make the font setting on a text object permanent is to not use the category names but to use an actual concrete font name. And ideally that should be provided as a list so that we can account for users who don't have a particular font installed.

I'm not sure what user code would fail because of this change -- if it does, I think it's probably a real corner case. I know there is a lot of code that sets font.family as a single name, but that will continue to work. But I think it's much less common that user code will read this value.

Owner

mdboom replied May 15, 2013

Ah -- I missed that about texmanager. The other text handling handled this just fine without further changes. I think maybe we should try to fix texmanager to support a list (see #2012).

Here the use case that brought this about:

The xkcd function needs to change the font of all text that is created while it is in effect, and then go back to the previous settings afterward. If you set font.family to fantasy and change font.fantasy to Humor Sans, this doesn't work, because the lookup from a category name ('fantasy') to a concrete name is dynamic so when font.family is reverted, so is all of the text that was created in the xkcd context. I think this is the correct behavior for category names, as they are supposed to be dynamically updatable. So the only way to make the font setting on a text object permanent is to not use the category names but to use an actual concrete font name. And ideally that should be provided as a list so that we can account for users who don't have a particular font installed.

I'm not sure what user code would fail because of this change -- if it does, I think it's probably a real corner case. I know there is a lot of code that sets font.family as a single name, but that will continue to work. But I think it's much less common that user code will read this value.

@leejjoon

This comment has been minimized.

Show comment Hide comment
@leejjoon

leejjoon May 21, 2013

@mdboom: is there any reason you deleted this line? __init__ method takes an explicit path_effects keyword parameter (it is not captured by kwargs), so path_effects argument is now ignored if one directly create Text instance. And this seems to be the cause of #2032. Same apply for Patch and Line2D. Either we resurrect these lines, or we modify the __init__ methods so that path_effects is captured by kwargs. Maybe the latter is what you meant?

@mdboom: is there any reason you deleted this line? __init__ method takes an explicit path_effects keyword parameter (it is not captured by kwargs), so path_effects argument is now ignored if one directly create Text instance. And this seems to be the cause of #2032. Same apply for Patch and Line2D. Either we resurrect these lines, or we modify the __init__ methods so that path_effects is captured by kwargs. Maybe the latter is what you meant?

@mdboom mdboom deleted the mdboom:wiggles branch Aug 7, 2014

@tacaswell tacaswell referenced this pull request Jul 8, 2017

Merged

Prefer to the GraphicsContext public API when possible. #8848

0 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment