Skip to content

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

Merged
merged 9 commits into from
Sep 8, 2021

Conversation

takimata
Copy link
Contributor

@takimata takimata commented Jun 25, 2021

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 give
control on how the line looks afterwards:

before

current

after

with same seed for both lines:

wanted

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

I only tested this with '3.0.2', though.

Copy link

@github-actions github-actions bot left a 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.

@tacaswell
Copy link
Member

attn @pwuertz

@tacaswell
Copy link
Member

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).

@tacaswell tacaswell added this to the v3.5.0 milestone Jun 25, 2021
Comment on lines 606 to 609
# 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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

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;
}

Copy link
Contributor Author

@takimata takimata Jul 20, 2021

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:

https://github.com/pgf-tikz/pgf/blob/247f1f0445a9e996560c9f5a9b3fb03a4184e6ec/tex/generic/pgf/libraries/decorations/pgflibrarydecorations.pathmorphing.code.tex#L86-L101

I didn't want to adapt matplotlib to pgf's behaviour more than necessary, so I just settled on this comment...

@jklymak jklymak marked this pull request as draft July 7, 2021 20:26
@jklymak jklymak marked this pull request as ready for review July 7, 2021 20:27
@jklymak
Copy link
Member

jklymak commented Jul 7, 2021

ooops, I mistakenly marked this draft, but apparently I cannot un-mark it (??). If you rebase it will clean up the tests.

@takimata
Copy link
Contributor Author

Yeah, hitting the wrong button in the web interface at the wrong time has all kinds of "interesting" effects...

@takimata
Copy link
Contributor Author

Ping.

I rewrote history to clean up my mess, hope nobody has used my branch yet.

@takimata
Copy link
Contributor Author

@QuLogic Rebased on master

@anntzer
Copy link
Contributor

anntzer commented Jul 23, 2021

  • It looks like you can use \usepgfmodule/\usepgflibrary locally, so I would just load the required pgf modules when emitting the paths with sketch params, so that this works relatively transparently for the end user.
  • I'm not sure this actually works? (Or the scales are very different?) e.g. xkcd(); plot([1, 2])[0].set_sketch_params(10, 128, 16); savefig("/tmp/test.png", backend="png") gives
    test
    i.e. the wobbliness is barely (if at all?) present?

@takimata
Copy link
Contributor Author

  • Like this?

  • Try a smaller length, e.g., 64 instead of 128.
    Unfortunately matplotlib and PGF require different parameters for the same/comparable image, e.g. scale=2, length=16, randomness=16 gives XKCD-like results with PGF (see pics below).

    Assuming that I correctly convert pixels to inch, I think this is caused by the different approach by PGF/matplotlib (see discussion above).
    Fixing this would require matplotlib/PGF to use the same algorithm to generate the wobbliness.

with plt.xkcd(scale=2, length=16, randomness=16); plt.plot([1, 2])[0].set_sketch_params(10, 64, randomness=16)

Matplotlib:
xkcd_mat

PGF:
xkcd_pgf

@anntzer
Copy link
Contributor

anntzer commented Jul 23, 2021

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.

@jklymak jklymak marked this pull request as draft July 27, 2021 13:54
@takimata
Copy link
Contributor Author

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.
So I think people would tend to accept a different behaviour...

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.

@tacaswell
Copy link
Member

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.

@takimata
Copy link
Contributor Author

@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

builtin
via_pgf

@takimata takimata marked this pull request as ready for review August 22, 2021 19:38
Copy link
Member

@QuLogic QuLogic left a 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.

takimata and others added 9 commits August 29, 2021 22:46
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>
@takimata
Copy link
Contributor Author

@QuLogic rebased to clean up the tests

@tacaswell
Copy link
Member

Thanks @takimata ! Congratulations on your first Matplotlib PR 🎉 Hopefully we will hear from you again!

QuLogic added a commit that referenced this pull request Sep 9, 2021
…518-on-v3.5.x

Backport PR #20518 on branch v3.5.x ( Support sketch_params in pgf backend)
@takimata
Copy link
Contributor Author

takimata commented Sep 9, 2021

Thanks for merging!

tacaswell added a commit that referenced this pull request Oct 20, 2021
ENH:  Support sketch_params in pgf backend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sketch params ignored when using PGF backend
5 participants