-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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.
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
Conversation
You're going to have to change the xkcd test image. See |
All reactions
Sorry, something went wrong.
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? |
All reactions
Sorry, something went wrong.
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! |
All reactions
Sorry, something went wrong.
Sorry, something went wrong.
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... |
All reactions
Sorry, something went wrong.
👍, but what did it look like on master? |
All reactions
Sorry, something went wrong.
You can see it attached in the issue #2625 |
All reactions
Sorry, something went wrong.
There was a problem hiding this 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.
Sorry, something went wrong.
All reactions
There was a problem hiding this 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.
Sorry, something went wrong.
All reactions
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... |
All reactions
Sorry, something went wrong.
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) |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dstansby, added it
Sorry, something went wrong.
All reactions
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. |
All reactions
Sorry, something went wrong.
The problem with the circle shape arises because of the path wiggle, so it does not close properly. |
All reactions
Sorry, something went wrong.
@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. |
All reactions
Sorry, something went wrong.
Re-milestoned to 3.1 as part of the 3.0 release triage. |
All reactions
Sorry, something went wrong.
@NelleV, thanks! I think I've done it in the last commit. Yet, I am not sure regarding next steps. |
All reactions
Sorry, something went wrong.
@TarasKuzyo We wait until CI has finished, and until @dstansby re-reviews this PR. |
All reactions
Sorry, something went wrong.
The pdf and svg files are still here on the other hand. |
All reactions
Sorry, something went wrong.
@NelleV, do you mean I had to delete pdfs and svgs for both xkcd tests? |
All reactions
Sorry, something went wrong.
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. |
All reactions
Sorry, something went wrong.
Great! Thanks @TarasKuzyo ! |
All reactions
Sorry, something went wrong.
timhoffm
jklymak
dstansby
Successfully merging this pull request may close these issues.
Markers in XKCD style
PR Summary
Partially fixes #2625 (Markers in XKCD style ).
I added missing
path.sketch
parameters when drawing markers inLine2D
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 forplt.scatter
as well.Unlike
plt.plot
makers inplt.scatter
are drawn withCollection
object (yet in both casesrenderer.draw_markers
is called). AFAUPathEffectRenderer
is required to display xkcd-style drawings properly butself._paths
(responsible for enabling path effects) is explicitly set toNone
inCollection
's constructor. When I tried commenting that line out, I got the following error message: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