-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Support sketch_params in pgf backend #20518
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this comment, as there's only one thing in amplitude
.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if (m_has_last) { | |
// We want the "cursor" along the sine wave to move at a | |
// random rate. | |
double d_rand = m_rand.get_double(); | |
double d_M_PI = 3.14159265358979323846; | |
m_p += pow(m_randomness, d_rand * 2.0 - 1.0); | |
double r = sin(m_p / (m_length / (d_M_PI * 2.0))) * m_scale; | |
double den = m_last_x - *x; | |
double num = m_last_y - *y; | |
double len = num * num + den * den; | |
m_last_x = *x; | |
m_last_y = *y; | |
if (len != 0) { | |
len = sqrt(len); | |
*x += r * num / len; | |
*y += r * -den / len; | |
} |
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.
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
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.
But you need to fix flake8.
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 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 givecontrol on how the line looks afterwards:
before
after
with same seed for both lines:
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and 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.