Added xkcd Style for Markers (plot only) by TarasKuzyo · Pull Request #11558 · matplotlib/matplotlib

TarasKuzyo · 2018-07-03T11:42:30Z

PR Summary

Partially fixes #2625 (Markers in XKCD style ).

I added missing path.sketch parameters when drawing markers in Line2D
with the following changes from xkcd defaults: (scale, length/2, 2*randomness).
It allows better sketch visibility on small-sized objects.

The functionality was requested for plt.plot only but I expected to add it for plt.scatter as well.
Unlike plt.plot makers in plt.scatter are drawn with Collection object (yet in both cases renderer.draw_markers is called). AFAU PathEffectRenderer is required to display xkcd-style drawings properly but self._paths (responsible for enabling path effects) is explicitly set to None inCollection's constructor. When I tried commenting that line out, I got the following error message:

File "/home/taras/Project/Activity/matplotlib/lib/matplotlib/patheffects.py", line 206, in draw_path
Stroke.draw_path(self, renderer, gc, tpath, affine, rgbFace)
File "/home/taras/Project/Activity/matplotlib/lib/matplotlib/patheffects.py", line 195, in draw_path
renderer.draw_path(gc0, tpath, trans, rgbFace)
File "/home/taras/Project/Activity/matplotlib/lib/matplotlib/backends/backend_agg.py", line 161, in draw_path
self._renderer.draw_path(gc, path, transform, rgbFace)
SystemError: new style getargs format but argument is not a tuple

It has something to do with incorrect argument passed to some internal function,
yet I am not sure how to proceed with it further.

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

jklymak · 2018-07-03T17:10:19Z

You're going to have to change the xkcd test image. See lib/matplotlib/tests/test_path.py

TarasKuzyo · 2018-07-03T18:47:10Z

Ok, thank you. Should I include a marker in the test case as well or should I rather write a separate test for a plot with markers?

jklymak · 2018-07-03T20:14:55Z

I assumed the test case had a marker or the test shouldn't have failed? Certainly if there was no marker in the old test, I'd go ahead and add one since you'll have to change the image anyway. Thanks!

TarasKuzyo · 2018-07-03T20:27:51Z

The old test case was just a plain line plot. Attaching the difference image

jklymak · 2018-07-03T20:31:02Z

OK< so the change you made caused those differences? They don't seem like a big deal to me, so I think its fine to put the changed image in. Up to you if you think adding more to that test makes sense or not. Does the xkcd example look OK after this? It'd be nice to see a before/after to know exactly what you've done here...

TarasKuzyo · 2018-07-03T20:33:53Z

Yes, still not sure where those changes come from. I will add one more test case then.
Attaching a sample plot with markers

jklymak · 2018-07-03T20:44:22Z

👍, but what did it look like on master?

TarasKuzyo · 2018-07-03T20:56:14Z

You can see it attached in the issue #2625

jklymak

LGTM, Thanks for the tests. We will likely ask that you squash commits before we merge.

timhoffm

Basically good. Optional wishlist:

I don't like the shape of the circles. They rather look like two shifted circles on top of each other (c.f. https://xkcd.com/1468/ - The circles should have a slightly deformed but smooth outline).
Is it possible to have individual variations per marker? Possibly not - I suspect that's part of the deal with plot.

jklymak · 2018-07-04T19:29:33Z

The circles should have a slightly deformed but smooth outline

I suspect that if one of the offset circles was made a different radius that would do the trick... Not sure how easy that is to impliment...

dstansby · 2018-07-05T12:50:45Z

        ax.plot(x, y)


+@image_comparison(baseline_images=['xkcd_marker'], remove_text=True)


Could you add extensions=['png'] to this bit, and only generate the .png images? It saves us cluttering up the repository with svg and pdf images.

@dstansby, added it

anntzer · 2018-07-06T09:22:48Z

I think(?) the trick for "better" circles is likely to play more with the sketch params, e.g. use something based on the markersize, or at least something even smaller than /2 (I agree with @timhoffm they don't look very good). Perhaps try some values and see if you can come up with something, although it's probably not worth overthinking it either.

TarasKuzyo · 2018-07-06T12:12:40Z

The problem with the circle shape arises because of the path wiggle, so it does not close properly.
Basically, start and end path nodes do not match. I am not sure if it is possible to come up with easy fix without breaking anything else.
Upd: It just occurred to me, that one can simply force end_node = start_node when drawing a closed path.

NelleV · 2018-07-07T17:00:18Z

@TarasKuzyo If I am not mistaken, the changes @dstansby asked you to make means you can delete the pdf and svg file.

It also means that whoever merges the pull request will have to use the squash and merge option to actually get rid of the binaries.

tacaswell · 2018-07-07T20:19:47Z

Re-milestoned to 3.1 as part of the 3.0 release triage.

TarasKuzyo · 2018-07-08T17:52:52Z

@NelleV, thanks! I think I've done it in the last commit. Yet, I am not sure regarding next steps.

NelleV · 2018-07-08T18:39:46Z

@TarasKuzyo We wait until CI has finished, and until @dstansby re-reviews this PR.

Comments addressed

NelleV · 2018-07-13T17:03:32Z

The pdf and svg files are still here on the other hand.

TarasKuzyo · 2018-07-28T18:36:40Z

@NelleV, do you mean I had to delete pdfs and svgs for both xkcd tests?

NelleV · 2018-07-28T22:56:15Z

I believe that the current test only rely on pngs, so yes, I'd remove the pdfs and svgs as they are not relevant anymore.

NelleV · 2018-07-29T16:13:18Z

Great! Thanks @TarasKuzyo !

TarasKuzyo added 2 commits July 3, 2018 13:26

added xkcd style for markers

1a5ee43

typo fix

47a3847

TarasKuzyo added 2 commits July 4, 2018 13:38

added a test case and baseline images for xkcd plot with markers

aedc80e

updated baseline images for test_xkcd

2d32050

jklymak approved these changes Jul 4, 2018

View reviewed changes

jklymak added this to the v3.0 milestone Jul 4, 2018

timhoffm reviewed Jul 4, 2018

View reviewed changes

dstansby previously requested changes Jul 5, 2018

View reviewed changes

generate only *.png images for xkcd_marker test

a6f6e3f

tacaswell modified the milestones: v3.0, v3.1 Jul 7, 2018

tacaswell modified the milestones: v3.1, v3.0 Jul 7, 2018

TarasKuzyo closed this Jul 8, 2018

TarasKuzyo reopened this Jul 8, 2018

TarasKuzyo added 2 commits July 29, 2018 09:08

removed svg and pdf tests for xkcd

f7d8d61

trailing whitespace

8fb0b8d

NelleV merged commit 31a8093 into matplotlib:master Jul 29, 2018

QuLogic modified the milestones: v3.1, v3.0 Jul 29, 2018

anntzer mentioned this pull request Jul 31, 2019

Add xkcd style file. #14943

Open

6 tasks

		ax.plot(x, y)


		@image_comparison(baseline_images=['xkcd_marker'], remove_text=True)

Uh oh!

Conversation

TarasKuzyo commented Jul 3, 2018

PR Summary

PR Checklist

Uh oh!

jklymak commented Jul 3, 2018

Uh oh!

TarasKuzyo commented Jul 3, 2018

Uh oh!

jklymak commented Jul 3, 2018

Uh oh!

TarasKuzyo commented Jul 3, 2018

Uh oh!

jklymak commented Jul 3, 2018

Uh oh!

TarasKuzyo commented Jul 3, 2018

Uh oh!

jklymak commented Jul 3, 2018

Uh oh!

TarasKuzyo commented Jul 3, 2018

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jul 4, 2018

Uh oh!

dstansby Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

TarasKuzyo Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

anntzer commented Jul 6, 2018

Uh oh!

TarasKuzyo commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NelleV commented Jul 7, 2018

Uh oh!

tacaswell commented Jul 7, 2018

Uh oh!

TarasKuzyo commented Jul 8, 2018

Uh oh!

NelleV commented Jul 8, 2018

Uh oh!

NelleV commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TarasKuzyo commented Jul 28, 2018

Uh oh!

NelleV commented Jul 28, 2018

Uh oh!

NelleV commented Jul 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

TarasKuzyo commented Jul 6, 2018 •

edited

Loading

NelleV commented Jul 13, 2018 •

edited

Loading