MNT: simplify valid tick logic #12158

jklymak · 2018-09-18T20:52:01Z

PR Summary

This is a much scaled back first-step PR for #12105.

The logic for deciding whether or not to draw a tick was rather convoluted, and depended on the size of a pixel. This PR simply changes the test for whether a tick is in the view limit to one that depends on a relative tolerance to take up floating point slack (like np.allclose, but for an interval). The relevant code is in transforms.inverval_contains.

Another minimal example (w/o resizing)

import matplotlib.pyplot as plt

fig, ax = plt.subplots()
ax.set_ylim(0, 1.199)

fig.savefig('boo100.png', dpi=100)
fig.savefig('boo200.png', dpi=200)

Before

100 dpi

200 dpi

After

100 dpi

200 dpi

As above...

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

jklymak · 2018-09-19T17:00:25Z

Ping @efiring

timhoffm · 2018-09-20T21:49:13Z

lib/matplotlib/axis.py

            if tick is None:
                continue
            # NB: always update labels and position to avoid issues like #9397
            tick.update_position(loc)
            tick.set_label1(label)
            tick.set_label2(label)
-            if not mtransforms.interval_contains(interval_expanded, loc):
+            try:
+                loct = self.get_transform().transform(loc)


Not sure if it really makes a performance difference, but AFAICS transform() works on numpy arrays. So this could be pulled out of the loop and calculated in one go. Well, except for the AssertionError - when does that occur?

We still have to do the loop, so can probably keep as-is for now.

I think the Assertion Error happened in CI, possibly get_transform returned None for some wacky axis.

Yeah its from the doc build: gallery/scales/custom_scale.py.

Exception occurred: File "/home/circleci/project/lib/matplotlib/transforms.py", line 1436, in transform assert not np.ma.is_masked(res) # just to be on the safe side AssertionError

The custom scale uses masks, and transforms.transform doesn't like it.

The previous implementation didn't call the transform, but this one does so that we can do the tolerance checking in log-scale rather than data space.

Are we sure that there are still any ticks on that example when we hit this?

eric-wieser · 2018-09-21T07:34:58Z

lib/matplotlib/transforms.py

@@ -2892,7 +2892,7 @@ def nonsingular(vmin, vmax, expander=0.001, tiny=1e-15, increasing=True):
    return vmin, vmax


-def interval_contains(interval, val):
+def interval_contains(interval, val, rtol=1e-10):


I feel like so as to not break downstream users relying on this being exact, the default should be rtol=0

@eric-wieser you probably understand the numerical issues better than me, but is this comparison ever "exact"?

Yes, comparison is always exact. The danger is when you make assumptions about rounding in operations prior to the comparison, but that's for the caller to decide.

OK, done...

lib/matplotlib/transforms.py

timhoffm · 2018-10-20T07:55:14Z

lib/matplotlib/axis.py

+            except AssertionError:
+                loct = None
+                continue
+            if ((loct is None) or


Can transform() return None? Otherwise we do not need the loct is None check here.

anntzer · 2018-11-05T09:31:40Z

lib/matplotlib/transforms.py

@@ -2906,10 +2906,39 @@ def interval_contains(interval, val):
    Returns
    -------
    bool
-        Returns true if given val is within the interval.
+        Returns true if given val is within the interval (with tolerance)


this one says nothing about tolerance

lib/matplotlib/transforms.py

tacaswell · 2018-11-19T20:21:46Z

We should make sure this does not break wcsaxes (attn @astrofrog )

anntzer · 2019-01-03T21:50:25Z

lib/matplotlib/axis.py

+                # some scales might make them, so we need this try/except.
+                loct = None
+                continue
+            if not mtransforms.interval_contains_close(inter, loct):


Do you have examples where we need interval_contains_close and not interval_contains here? Why couldn't the locators be in charge of making sure whatever they return is in the interval and adjusting the values if needed (except for those outside the interval needed for classic mode)?

I don't really like the idea of hardcoding the 1e-10 tolerance here when it's something that can clearly(?) be avoided.

Hah, we currently have a 1/2-pixel tolerance! Surely 1e-10 * interval is preferable?

The problem with limits is when people compute them.

On my machine:

3.51-1.51 == 2.0 False In [17]: abs(3.51-1.51- 2.0) < 1e-10 Out[17]: True

I guess it doesn't make things worse, but at least let's keep interval_contains_close private?

The point is that the Locator API should say something like the following to implementers (including 3rd party locators):

You may return tick values inside or outside of the axis bounds, but any value outside of the bounds (with no tolerance) will be dropped (except in "classic" mode, where we will keep the first outside value on either side).

and the locator classes are the one which should do something like

xmin, xmax = ... ticks = ... if xmin - tol < ticks[0] < xmin: ticks[0] = xmin

(or ticks[1] if that's the one corresponding to xmin).
i.e. the locator displaces the tick to match xmin if it's just below xmin.

But I guess that can wait.

The issue with locators is that some uses want the bracketing ticks, unfortunately, for contour for instance... We could fix/change that as well, but beyond this PR.

Agreed it's beyond this PR.
The design above does not preclude bracketing ticks.

@anntzer did you really want interval_contains_close to be private? I actually think its useful to take into account floating point slop, but don't feel strongly about it.

yes, it feels like a hack while waiting for locators to be fixed, but I'm not going to insist on it.

Okey doke - done!

FIX: make contains_close private

timhoffm

This looks ok by itself.

Not merging yet because of @tacaswell's concern with respect to wcsaxes. Can someone confirm, that this is not breaking them?

astrofrog · 2019-01-08T13:54:16Z

I'm checking it and will report back shortly

astrofrog

This works fine with WCSAxes (all tests, including image tests, pass), in part because we do all the tick drawing ourselves.

timhoffm · 2019-01-08T18:48:51Z

@astrofrog Thanks for testing.

jklymak · 2019-01-08T19:00:07Z

Yes, thanks all!

jklymak force-pushed the mnt-simplify-validtick branch from 157b0e1 to f49d364 Compare September 18, 2018 21:55

jklymak added this to the v3.1 milestone Sep 19, 2018

timhoffm reviewed Sep 20, 2018

View reviewed changes

eric-wieser reviewed Sep 21, 2018

View reviewed changes

jklymak mentioned this pull request Oct 19, 2018

Saving plot as PNG file prunes tick labels #12564

Closed

jklymak force-pushed the mnt-simplify-validtick branch from e2a1a63 to 61ea269 Compare October 19, 2018 15:14

eric-wieser reviewed Oct 19, 2018

View reviewed changes

lib/matplotlib/transforms.py Show resolved Hide resolved

timhoffm reviewed Oct 20, 2018

View reviewed changes

jklymak force-pushed the mnt-simplify-validtick branch from 98f1560 to 2ee3e6d Compare October 20, 2018 21:29

jklymak mentioned this pull request Nov 5, 2018

FIX: properly set tz for YearLocator #12678

Merged

6 tasks

jklymak force-pushed the mnt-simplify-validtick branch from 2ee3e6d to 3d4976b Compare November 5, 2018 03:10

anntzer reviewed Nov 5, 2018

View reviewed changes

timhoffm reviewed Nov 17, 2018

View reviewed changes

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

jklymak force-pushed the mnt-simplify-validtick branch from c6df889 to 227bed5 Compare November 17, 2018 22:00

jklymak added topic: ticks axis labels status: needs review labels Jan 3, 2019

anntzer reviewed Jan 3, 2019

View reviewed changes

MNT: simplify valid tick logic

01d7455

FIX: make contains_close private

jklymak force-pushed the mnt-simplify-validtick branch from 227bed5 to 01d7455 Compare January 4, 2019 17:08

anntzer approved these changes Jan 4, 2019

View reviewed changes

timhoffm approved these changes Jan 6, 2019

View reviewed changes

astrofrog approved these changes Jan 8, 2019

View reviewed changes

timhoffm merged commit d23bcea into matplotlib:master Jan 8, 2019

jklymak deleted the mnt-simplify-validtick branch January 8, 2019 19:00

anntzer removed the status: needs review label Jan 8, 2019

anntzer mentioned this pull request Feb 5, 2019

Kill the unused, private _get_pixel_distance_along_axis. #13362

Merged

6 tasks

jklymak mentioned this pull request Feb 26, 2019

Deprecate smart_bounds handling in Axis and Spine #11004

Merged

6 tasks

jklymak mentioned this pull request Nov 29, 2019

Fixes pyplot xticks() and yticks() by allowing setting only the labels #15788

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT: simplify valid tick logic #12158

MNT: simplify valid tick logic #12158

jklymak commented Sep 18, 2018 •

edited

Loading

jklymak commented Sep 19, 2018

timhoffm Sep 20, 2018

jklymak Oct 19, 2018

jklymak Oct 20, 2018

tacaswell Nov 19, 2018

eric-wieser Sep 21, 2018

jklymak Sep 22, 2018

eric-wieser Sep 22, 2018

jklymak Oct 19, 2018

timhoffm Oct 20, 2018

anntzer Nov 5, 2018

jklymak Nov 19, 2018

tacaswell commented Nov 19, 2018

anntzer Jan 3, 2019 •

edited

Loading

jklymak Jan 3, 2019

anntzer Jan 3, 2019 •

edited

Loading

jklymak Jan 3, 2019

anntzer Jan 3, 2019

jklymak Jan 4, 2019

anntzer Jan 4, 2019

jklymak Jan 4, 2019

timhoffm left a comment •

edited

Loading

astrofrog commented Jan 8, 2019

astrofrog left a comment

timhoffm commented Jan 8, 2019 •

edited

Loading

jklymak commented Jan 8, 2019

MNT: simplify valid tick logic #12158

MNT: simplify valid tick logic #12158

Conversation

jklymak commented Sep 18, 2018 • edited Loading

PR Summary

Another minimal example (w/o resizing)

Before

100 dpi

200 dpi

After

100 dpi

200 dpi

PR Checklist

jklymak commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Nov 19, 2018

anntzer Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

astrofrog commented Jan 8, 2019

astrofrog left a comment

Choose a reason for hiding this comment

timhoffm commented Jan 8, 2019 • edited Loading

jklymak commented Jan 8, 2019

jklymak commented Sep 18, 2018 •

edited

Loading

anntzer Jan 3, 2019 •

edited

Loading

anntzer Jan 3, 2019 •

edited

Loading

timhoffm left a comment •

edited

Loading

timhoffm commented Jan 8, 2019 •

edited

Loading