Skip to content

Added xkcd Style for Markers (plot only) #11558

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 29, 2018
Merged

Added xkcd Style for Markers (plot only) #11558

merged 7 commits into from
Jul 29, 2018

Conversation

TarasKuzyo
Copy link
Contributor

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
Copy link
Member

jklymak commented Jul 3, 2018

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

@TarasKuzyo
Copy link
Contributor Author

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
Copy link
Member

jklymak commented Jul 3, 2018

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
Copy link
Contributor Author

The old test case was just a plain line plot. Attaching the difference image
xkcd-failed-diff

@jklymak
Copy link
Member

jklymak commented Jul 3, 2018

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
Copy link
Contributor Author

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

@jklymak
Copy link
Member

jklymak commented Jul 3, 2018

👍, but what did it look like on master?

@TarasKuzyo
Copy link
Contributor Author

You can see it attached in the issue #2625

jklymak
jklymak approved these changes Jul 4, 2018
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

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

@jklymak jklymak added this to the v3.0 milestone Jul 4, 2018
timhoffm
timhoffm reviewed Jul 4, 2018
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

jklymak commented Jul 4, 2018

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
dstansby previously requested changes Jul 5, 2018
View reviewed changes
lib/matplotlib/tests/test_path.py Outdated
@@ -122,6 +122,22 @@ def test_xkcd():
ax.plot(x, y)


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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dstansby, added it

@anntzer
Copy link
Contributor

anntzer commented Jul 6, 2018

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
Copy link
Contributor Author

TarasKuzyo commented Jul 6, 2018 •
edited
Loading

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
Copy link
Member

NelleV commented Jul 7, 2018

@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 tacaswell modified the milestones: v3.0, v3.1 Jul 7, 2018
@tacaswell
Copy link
Member

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

@tacaswell tacaswell modified the milestones: v3.1, v3.0 Jul 7, 2018
@TarasKuzyo
Copy link
Contributor Author

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

@TarasKuzyo TarasKuzyo closed this Jul 8, 2018
@TarasKuzyo TarasKuzyo reopened this Jul 8, 2018
@NelleV
Copy link
Member

NelleV commented Jul 8, 2018

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

@dstansby dstansby dismissed their stale review July 9, 2018 03:13

Comments addressed

@NelleV
Copy link
Member

NelleV commented Jul 13, 2018 •
edited
Loading

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

@TarasKuzyo
Copy link
Contributor Author

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

@NelleV
Copy link
Member

NelleV commented Jul 28, 2018

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
Copy link
Member

NelleV commented Jul 29, 2018

Great! Thanks @TarasKuzyo !

@NelleV NelleV merged commit 31a8093 into matplotlib:master Jul 29, 2018
@QuLogic QuLogic modified the milestones: v3.1, v3.0 Jul 29, 2018
@anntzer anntzer mentioned this pull request Jul 31, 2019
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm left review comments

@jklymak jklymak jklymak approved these changes

@dstansby dstansby dstansby left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

Markers in XKCD style
8 participants
@TarasKuzyo @jklymak @anntzer @NelleV @tacaswell @timhoffm @dstansby @QuLogic