Don't let margins expand polar plots to negative radii by default. by anntzer · Pull Request #13980 · matplotlib/matplotlib · GitHub

This is implemented by altering the semantics of sticky edges as
documented in the changelog. As it turns out, this change also improves
consistency for streamplot():

in test_streamplot.py::test_{linewidth,mask_and_nans}, the small bump
in tolerance is necessary because the axes limits are now (-3, 3)
(matching the vector field limits), whereas they were previously
xlim=(-3.0, 2.9999999999999947), ylim=(-3.0000000000000004, 2.9999999999999947)
(they can be inspected with
plt.gcf().canvas.draw(); print(plt.gca().get_xlim(), plt.gca().get_ylim())).
in test_streamplot.py::test_maxlength, note that the previous version
expanded the axes limits beyond (-3, 3), whereas the current doesn't
do so anymore. It doesn't actually make much sense if the vector
field limits are applied if the streamplot goes all the way to the
edges, but are ignored otherwise.

Closes #13292.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 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

Why bump the tolerance instead of replacing the test image?

see https://gitter.im/matplotlib/matplotlib?at=5c87e610ac408e11924acaa9

We definitely shouldn't bump tolerances because the actual test image has changed, as this means any small changes to the images in future will not be picked up.

Sure, I just forced the axes limits to their old values instead...

What is the 1e-8 doing for us here that the 1e-5*abs(x0) is not?

x0 could be equal to zero and we would still need some slop... the actual value comes from what isclose() uses, I didn't really want to bother finding something better (really the correct thing to do should be to fix streamplot() so that we don't need slop at all, but that's a separate project).

Can you factor that out into a function? This is used twice and the function removes the magic numbers.

def tol(x, rtol=1e-5, atol=1e-8):
    """A tolerance for x such that x+/-tol is considered close to x."""
    return rtol * abs(x) + atol

That doesn't really work because in one case it's plus and in the other it's minus. I moved the values to their own variables, though.

But that would just be x0 + tol(x0) and x1 - tol(x1) in the code.

sure, pushed

Is it a problem that these are now the same object? I assume we do not mutate these values below?

Technically, this is probably working. However, I'd prefer

x_stickies = np.array([])
y_stickies = np.array([])

First, we don't know if these might be written to for some reason in the future. Second, it keeps the symmetry.

I moved the conditional higher to avoid this need.

Left one comment about the numerics of 'close'.

I also reverted the sole image change by instead adding a check that the "correct limits" are applied, and then forcing the limits to the old values.

This is implemented by altering the semantics of sticky edges as documented in the changelog. As it turns out, this change also improves consistency for streamplot(): - in test_streamplot.py::test_{linewidth,mask_and_nans}, the change is necessary because the axes limits would now be (-3, 3) (matching the vector field limits), whereas they were previously xlim=(-3.0, 2.9999999999999947), ylim=(-3.0000000000000004, 2.9999999999999947) (they can be inspected with `plt.gcf().canvas.draw(); print(plt.gca().get_xlim(), plt.gca().get_ylim())`). - in test_streamplot.py::test_maxlength, note that the previous version expanded the axes limits *beyond* (-3, 3), whereas the current doesn't do so anymore. It doesn't actually make much sense if the vector field limits are applied if the streamplot goes all the way to the edges, but are ignored otherwise.

anntzer added the topic: polar label Apr 18, 2019

dstansby reviewed Apr 18, 2019

View reviewed changes

anntzer force-pushed the polarzero branch from a70380e to f0da0cf Compare April 18, 2019 22:46

tacaswell reviewed Apr 20, 2019

View reviewed changes

tacaswell approved these changes Apr 20, 2019

View reviewed changes

tacaswell added this to the v3.2.0 milestone Apr 20, 2019

anntzer force-pushed the polarzero branch 2 times, most recently from cce31fa to 0a035aa Compare May 4, 2019 15:50

anntzer force-pushed the polarzero branch from 0a035aa to 3c530df Compare May 4, 2019 15:52

anntzer force-pushed the polarzero branch from 3c530df to 0911f8a Compare May 4, 2019 21:31

timhoffm approved these changes May 4, 2019

View reviewed changes

timhoffm merged commit 474f6c5 into matplotlib:master May 4, 2019

anntzer deleted the polarzero branch May 4, 2019 22:52

timhoffm mentioned this pull request Dec 2, 2019

Polar plot autoscaling changes radial bound to negative. #15814

Closed

jklymak mentioned this pull request Jun 15, 2020

Stackplot fails for small numbers #17633

Closed

jklymak mentioned this pull request Oct 29, 2021

[Bug]: Scatter Plot Returns Different Plots in Different Versions #21482

Closed

anntzer mentioned this pull request Jun 14, 2024

Make sticky edges only apply if the sticky edge is the most extreme limit point #28393

Merged

5 tasks done

               @image_comparison(baseline_images=['streamplot_masks_and_nans'],
-                                tol=0.04 if on_win else 0,
-                                remove_text=True, style='mpl20')
+                                tol=.1, remove_text=True, style='mpl20')

+                          # tolerances (whose value come from isclose()) must be used due to
+                          # floating point issues with streamplot.
+                          # Index of largest element < x0 + tol, if any.
+                          i0 = stickies.searchsorted(x0 + 1e-5 * abs(x0) + 1e-8) - 1

                               y_stickies = y_stickies[y_stickies > 0]
                       else:  # Small optimization.
-                          x_stickies, y_stickies = [], []
+                          x_stickies = y_stickies = np.array([])

Don't let margins expand polar plots to negative radii by default. by anntzer · Pull Request #13980 · matplotlib/matplotlib · GitHub

Repository navigation

Don't let margins expand polar plots to negative radii by default. - #13980

Don't let margins expand polar plots to negative radii by default.#13980timhoffm merged 1 commit intomatplotlib:mastermatplotlib/matplotlib:masterfrom anntzer:polarzeroanntzer/matplotlib:polarzeroCopy head branch name to clipboard

Conversation

anntzer commented Apr 18, 2019

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented May 4, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Don't let margins expand polar plots to negative radii by default.#13980
timhoffm merged 1 commit into
matplotlib:masterfrom
anntzer:polarzero

anntzer Apr 23, 2019 •
edited
Loading

timhoffm May 4, 2019 •
edited
Loading