Support sketch_params in pgf backend#20518
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
attn @pwuertz |
|
Thank you for working on this @takimata ! This will definitely need a test (to make sure we do not accidentally break it in the future). |
| # Only "length" directly maps to "segment length" in PGF's API | ||
| # The others are combined in "amplitude" -> Use "randomness" as | ||
| # PRNG seed to allow the user to force the same shape on multiple | ||
| # sketched lines |
There was a problem hiding this comment.
I'm confused by this comment, as there's only one thing in amplitude.
There was a problem hiding this comment.
Well, matplotlib: https://matplotlib.org/stable/api/_as_gen/matplotlib.artist.Artist.set_sketch_params.html and pgf: https://mirror.physik.tu-berlin.de/pub/CTAN/graphics/pgf/base/doc/pgfmanual.pdf#subsubsection.50.3.1 (page 640, "Decoration random steps") do not exactly fit together.
PGF only has "segment length" and "amplitude" as arguments (randomness coming from a built-in RNG).
"amplitude" changes both the length of the wiggle along the line AND the wiggle perpendicular to the source line.
From my understanding, the matplotlib docs say that it only varies the "(segment) length" by a given randomized factor of "randomness" (without randomizing the wiggle perpendicular to the source line).
I updated the comment, should be more clear now.
There was a problem hiding this comment.
The length defines the wavelength of the sine wave used for perturbation. The randomness determines how quickly each segment goes through the sine wave. So I guess they are kind of two arguments to the same thing.
See
matplotlib/src/path_converters.h
Lines 978 to 994 in 246a489
There was a problem hiding this comment.
Yes, and the real amplitude (as in a 1D sine wave) is not randomized by randomness (which could be considered as a separate bug, imho).
PGF uses a completely different approach (and a 2D amplitude):
The line is split into segments of length segment length which walk towards the destination point.
The end coordinate of each line segment (aka step) is randomized by at most ± amplitude, independently in both X- and Y direction:
I didn't want to adapt matplotlib to pgf's behaviour more than necessary, so I just settled on this comment...
|
ooops, I mistakenly marked this draft, but apparently I cannot un-mark it (??). If you rebase it will clean up the tests. |
|
Yeah, hitting the wrong button in the web interface at the wrong time has all kinds of "interesting" effects... |
1a9fb1e to
a68b7f4
Compare
|
Ping. I rewrote history to clean up my mess, hope nobody has used my branch yet. |
a68b7f4 to
4763798
Compare
|
@QuLogic Rebased on master |
with |
|
I don't think the algorithms have to match exactly, but I think it would be preferable for the pgf backend to scale the numbers in whatever way needed to make the visual impression similar for similar values of sketch_params. |
|
I agree from a user perspective. But in the end it would be an ugly abstraction which breaks in some cases and which always breaks the documentation in a way that the units given there ("length in pixels") are simply not correct. When using the PGF backend at all, my only use case for the matplotlib rendering was a (rough) preview, because the PDF produced by LaTeX doesn't exactly match matplotlib's PDF anyway. For merging this PR, such a workaround could be ok, but in the long term I would prefer having this as a separate feature. I.e., PGF-like sketched lines as a strategy for set_sketch_params. |
|
We talked about this on the call today and the consensus is that it is OK if the wiggles are different (as you would expect because they are implemented differently under the hood), but at least the amplitude should be scale to look ball-park the same. |
5673694 to
4a19a38
Compare
|
@tacaswell Thanks for discussing and deciding this. I've added some scaling which should work for most cases. import matplotlib as mpl
import matplotlib.pyplot as plt
mpl.rcParams.update({
'pgf.texsystem': "pdflatex",
'figure.figsize': [4, 3],
})
plt.xkcd(scale=1, length=100, randomness=8)
plt.plot([1, 2])[0].set_sketch_params(scale=10, length=32, randomness=16)
plt.tight_layout()
plt.savefig("builtin.png")
plt.savefig("via_pgf.png", backend="pgf")now yields |
4a19a38 to
08d1aa6
Compare
Fixes matplotlib#20516 PGF's `random steps` decoration seems to be the most similar, but does not exactly match the behaviour described in matplotlib's docs. Therefore I repurposed the `randomness` argument as a seed to give control on how the line looks afterwards.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
5d95839 to
c3503a2
Compare
|
@QuLogic rebased to clean up the tests |
|
Thanks @takimata ! Congratulations on your first Matplotlib PR 🎉 Hopefully we will hear from you again! |
…518-on-v3.5.x Backport PR #20518 on branch v3.5.x ( Support sketch_params in pgf backend)
|
Thanks for merging! |
ENH: Support sketch_params in pgf backend





PR Summary
Fixes #20516
PGF's
random stepsdecoration seems to be the most similar,but does not exactly match the behaviour described in matplotlib's docs.
Therefore I repurposed the
randomnessargument as a seed to givecontrol on how the line looks afterwards:
before
after
with same seed for both lines:
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).I only tested this with '3.0.2', though.