Move major/minor tick overstrike logic to Axis. #13314

anntzer · 2019-01-29T11:04:03Z

PR Summary

The Axis knows what the major and the minor ticker are so it makes sense
to let it handle the deduplication logic.

Revert some heuristic deduplication logic from the Locators themselves
(which effectively tests that the new code works). (#11762, #13126)

Closes #11575 (which has a few duplicates).

Goes on top of #12909 (which has already three approvals). [edit: that got merged]

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

anntzer · 2019-01-29T14:27:37Z

The test_tightlayout.py::test_outward_ticks failure is explicitly testing that one can overlay a minor tick on top of a major tick, so I'll probably just remove that test (after looking into whether it is testing anything else relevant) (The patch below does not help because the test even requires a major label at x=0 that's overstruck by a minor tick:

diff --git i/lib/matplotlib/tests/test_tightlayout.py w/lib/matplotlib/tests/test_tightlayout.py
index 5898c1a94..a8f833f07 100644
--- i/lib/matplotlib/tests/test_tightlayout.py
+++ w/lib/matplotlib/tests/test_tightlayout.py
@@ -167,16 +167,14 @@ def test_outward_ticks():
     'Test automatic use of tight_layout'
     fig = plt.figure()
     ax = fig.add_subplot(221)
-    ax.xaxis.set_tick_params(tickdir='out', length=16, width=3)
-    ax.yaxis.set_tick_params(tickdir='out', length=16, width=3)
-    ax.xaxis.set_tick_params(
-        tickdir='out', length=32, width=3, tick1On=True, which='minor')
-    ax.yaxis.set_tick_params(
-        tickdir='out', length=32, width=3, tick1On=True, which='minor')
-    # The following minor ticks are not labelled, and they
-    # are drawn over the major ticks and labels--ugly!
-    ax.xaxis.set_ticks([0], minor=True)
-    ax.yaxis.set_ticks([0], minor=True)
+    for axis in [ax.xaxis, ax.yaxis]:
+        axis.set_ticks([0.2, 0.4, 0.6, 0.8, 1.0])
+        axis.set_tick_params(tickdir='out', length=16, width=3)
+        axis.set_ticks([0], minor=True)
+        # The following minor ticks are not labelled, and they
+        # are drawn over the major ticks and labels--ugly!
+        axis.set_tick_params(
+            which='minor', tick1On=True, tickdir='out', length=32, width=3)
     ax = fig.add_subplot(222)
     ax.xaxis.set_tick_params(tickdir='in', length=32, width=3)
     ax.yaxis.set_tick_params(tickdir='in', length=32, width=3)

)

The test_scale.py::test_logscales failure is (I believe?) due to the fact that the Symlog minor locator previously did not implement overstriking avoidance, so we ended up with overstruck major and minor ticks, which the svg rasterizer would pick up (one can examine the svg output to confirm); with this PR, the overstruck minor tick is absent, causing a tiny change in rasterization. Again, may be worth just killing the test if it's not testing anything else (after proper investigation).

Edit: test_logscales was introduced in #1247 to check that ax{v,h}line works with (sym)log scales. Rewrote it to use check_figures_equal.

jklymak · 2019-01-29T15:12:05Z

Overall 👍 But another approach would have been to add a remove_ticks kwarg to locator.call. Not sure which approach is better but this way the locator could decide what to do with the ticks to remove. It’s also possible that the locator logic could do something interesting with the major ticks info.

anntzer · 2019-01-29T15:57:49Z

Adding a remove_ticks=... to __call__ means going through a deprecation to not break third-party locators (ugh), and likely a bunch of locators will need to basically use the same code anyways (well, that can always be factored out in a helper method, but still...)

OTOH it does allow for really weird cases if you really want your custom locator to overstrike a major tick with a minor tick, not that I think there's a realistic use case... (the test_outwards_tick test is completely artificial.)

jklymak · 2019-01-29T16:53:38Z

Oh, duh, you are right - I thought calling foo(unknown_kwarg='Boo') just ignored the kwarg, but thats only if the call signature has **kwargs which locator.__call__ doesn't have.

Now that I think of it, allowing API changes to have better backwards compatibility is actually a pretty good reason to have signatures with **kwargs.

jklymak · 2019-01-29T16:55:57Z

For the "weird" cases, I could imagine knowing the major ticks could change the minor tick intervals. i.e. for dates, if you know the major ticks are days, making the minor ticks hours is useful. No doubt we already have logic for that, but it would be made easier by knowing the major ticks.

I guess compatibility could be maintained by a try block, but that is inelegant.

anntzer · 2019-01-29T17:02:27Z

Now that I think of it, allowing API changes to have better backwards compatibility is actually a pretty good reason to have signatures with **kwargs.

That doesn't really help: for our own locators we can update them as desired, for third party locators we can't really force them to accept **kwargs as they would work fine without that.
A try: block introspecting the signature is probably doable, just a minor pain.

For the "weird" cases, I could imagine knowing the major ticks could change the minor tick intervals. i.e. for dates, if you know the major ticks are days, making the minor ticks hours is useful. No doubt we already have logic for that, but it would be made easier by knowing the major ticks.

Should probably have a look at what adhoc mechanisms are currently used and how this PR could or could not subsume them.

anntzer · 2019-01-29T21:34:23Z

@jklymak I skimmed through dates.py and didn't see anything noticeable. Do you know of any example with interesting interactions between major and minor dates locators?

jklymak · 2019-01-29T21:49:24Z

No I don’t think so, but you could imagine doing something like that....

anntzer · 2019-01-29T21:59:04Z

Then I think I'd rather leave this PR as it is and worry about changing the signature of __call__ when an actual use case shows up...

anntzer · 2019-01-29T22:20:36Z

Edited the other failing test (initially added in #5683) to check the axes positions instead.

anntzer · 2019-01-29T22:49:29Z

Missed the fact that test_symlog{,2} suffers from the same overstriking problem as test_logscales. Dropped the png and svg tests, and kept the pdf baseline, as ghostscript appears not to pick up the overstriking, so the fundamental functionality (of symlog scales) is still tested.

jklymak · 2019-01-29T22:51:48Z

I don't think there is a compelling need to have those tests make pdfs and svgs. OTOH, I bet the png still won't match because of anti-aliasing so maybe update the baseline image?

anntzer · 2019-01-29T22:53:16Z

the png also fails due to antialiasing (likely), it is the pdf test that I kept; likely it works because ghostscript's rasterizer is not as good and does not perform the antialiasing correctly? :p

PS: I find it pretty funny that github's UI includes the svg files in the +/- lines count :)

anntzer · 2019-01-29T23:50:40Z

Thinking again about adding remove_ticks to __call__ as a mechanism for inter-locator communication, I think that basically assumes (*) that the minor locator would always be used with the "corresponding" major locator (so that the minor locator can meaningfully interpret what is pass down to it). It is true that in general, a minor loglocator is used with a major loglocator (for example). But if you look at the issue that I mentioned in this PR, some of them come from people forcing major tick locations (effectively using a FixedLocator under the hood) and expecting the minor ticker to know not to put a tick there, so the assumption (*) does not hold.

If a minor locator really wants to know what a major locator did, it should probably(?) do something like 1) check that itself is the minor locator (self is self.axis.minor.locator) and then 2) call the major locator again to see what it outputs (self.axis.major.locator()).

jklymak · 2019-01-29T23:58:32Z

I don't follow that argument. Why would it assume anything about what kind of locator it is or if its been used with a major locator? Its just saying: don't use these possible tick values, and maybe being clever about what to do between those tick values.

If you like, we could call it major_ticks and then that is the signal that the locator is a minor locator.

anntzer · 2019-01-30T00:15:06Z

Again, I think we should have an actual example where passing the major ticks down is useful before going through the API change pain.
Also, self is self.axis.minor.locator is such an easy way to check whether you're the minor locator that I wonder why we missed it before [edit: I guess because that doesn't work for locators not associated with an axis, but I'm not sure we really care about that case...].

jklymak · 2019-01-30T01:10:34Z

OK, getting in the weeds, but why does the locator care about its axis? We pass stuff around on a somewhat ad-hoc basis and this is one case where I'm not sure what the benefit is.

anntzer · 2019-01-30T10:03:58Z

Right now, the locators need to know what the axis is because when you call locator(), they need to look up the axis limits; they also use it to consult axis.get_tick_space() (for determining the number of ticks).

The formatters need to know what the axis is to consult the axis limits.

I would like to make tickers independent of the axis (and always pass the axis down as argument) to make them reusable over multiple axises, but that's not going to happen here.

jklymak

Just commenting - these aren't necessarily blockers from my point of view, but should be discussed before I approve....

doc/api/next_api_changes/2018-01-30-AL.rst

lib/matplotlib/axis.py

jklymak · 2019-01-30T17:17:09Z

lib/matplotlib/axis.py

-        return self.minor.locator()
+        """Get the array of minor tick locations in data coordinates."""
+        # Remove minor ticks duplicating major ticks.
+        major_locs = self.major.locator()


why not pass the major_locs here, if we don't want to do it in the locator? Agree that its an API change, but I suspect a minor one; this method is not used elsewhere, except in the tests...

Because if anything I think the main API change here is that one cannot call axis.minor.locator() to get the minor tick locations. The main API to do that is now axis.get_minorticklocs(); making it axis.get_minorticklocs(major_locs=axis.get_majorticklocs()) seems a bit of a pain (Also, what about axis.get_ticklocs(minor=True)? Would you now have to pass the major_locs kwarg to get_ticklocs() but only if minor=True? Looks a bit awkward.).

jklymak · 2019-01-30T17:19:03Z

lib/matplotlib/axis.py

+        """Get the array of minor tick locations in data coordinates."""
+        # Remove minor ticks duplicating major ticks.
+        major_locs = self.major.locator()
+        minor_locs = self.minor.locator()


aren't we now calling both locators twice? Seems inefficient to me.

Yes (per the above).
Frankly, would not worry as I doubt it's anywhere close to being a bottleneck, but we could factor out get_minorticklocs to a private helper that takes majorlocs as parameter if this is really an issue...

OK, actually I'm confused - why are we calling the locator at all here? Do we not cache the existing tick locations in some way? Sorry if I'm being dumb.

I... don't think so? Note that the previous implementation also called the locator.
Obviously we could inspect what are the current tick positions, but I don't know if there's any invalidation mechanism in place right now (e.g. what happens when you pan/zoom?).
IOW, this uses the same structure as before the PR; having get_{major,minor}ticklocs handle cache invalidation could be future work ( :) ).

anntzer · 2019-02-12T13:23:23Z

Tentatively milestoning as 3.1 as this already has one positive review (and fixes a longstanding bug), feel free to remilestone if it doesn't make it.

timhoffm · 2019-02-16T20:22:08Z

lib/matplotlib/axis.py

+        lo, hi = sorted(transform.transform(self.get_view_interval()))
+        # Use the transformed view limits as scale.  1e-5 is the default rtol
+        # for np.isclose.
+        tol = (hi - lo) * 1e-5


"transformed view limits" means this is now in axes coordinates, right? If so, this is the right thing to do.

It's some linear transform away from axes coordinates, so the relative check is equivalent to being done in axes coordinates.

The Axis knows what the major and the minor ticker are so it makes sense to let it handle the deduplication logic. Revert some heuristic deduplication logic from the Locators themselves (which effectively tests that the new code works).

Its original intent was to check that ax{v,h}line works in (sym)log scales, and the baseline svg changed with the removal of minor tick overstriking.

anntzer · 2019-02-16T20:35:13Z

rebased

timhoffm · 2019-02-16T21:11:23Z

Anyone can merge after CI pass.

tacaswell · 2019-02-23T16:58:15Z

azure failure was download related, restarting.

…to Axis.

…314-on-v3.1.x Backport PR #13314 on branch v3.1.x (Move major/minor tick overstrike logic to Axis.)

In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.

If Axis.remove_overlapping_locs is set to False the `Axis` object will not attempt to remove locations in the minor ticker that overlap with locations in the major ticker. This is a follow up to matplotlib#13314 which un-conditionally removed the overlap. closes matplotlib#13618

In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.

If Axis.remove_overlapping_locs is set to False the `Axis` object will not attempt to remove locations in the minor ticker that overlap with locations in the major ticker. This is a follow up to matplotlib#13314 which un-conditionally removed the overlap. closes matplotlib#13618

anntzer added topic: ticks axis labels status: waiting for other PR labels Jan 29, 2019

anntzer force-pushed the minoroverstrike branch 2 times, most recently from 35c95de to a67157d Compare January 29, 2019 13:08

anntzer force-pushed the minoroverstrike branch from 11394cf to 638bdd0 Compare January 29, 2019 16:25

anntzer removed the status: waiting for other PR label Jan 29, 2019

jklymak reviewed Jan 30, 2019

View reviewed changes

anntzer force-pushed the minoroverstrike branch from 532ad38 to d953c0b Compare January 30, 2019 18:28

jklymak approved these changes Feb 1, 2019

View reviewed changes

anntzer mentioned this pull request Feb 5, 2019

Inline iter_ticks into _update_ticks, and use that in mplot3d. #13363

Merged

6 tasks

anntzer added this to the v3.1.0 milestone Feb 12, 2019

timhoffm approved these changes Feb 16, 2019

View reviewed changes

timhoffm added the status: needs rebase label Feb 16, 2019

anntzer added 5 commits February 16, 2019 21:32

Move major/minor tick overstrike logic to Axis.

97eceb1

The Axis knows what the major and the minor ticker are so it makes sense to let it handle the deduplication logic. Revert some heuristic deduplication logic from the Locators themselves (which effectively tests that the new code works).

Update test_log_scales to use check_figures_equal.

07ac300

Its original intent was to check that ax{v,h}line works in (sym)log scales, and the baseline svg changed with the removal of minor tick overstriking.

Update test_outward_ticks to check for axes positions after tightlayout.

7c201fd

Remove png/svg versions of test_symlog{,2}.

dbc693a

Clarify a bit iter_ticks.

a752d46

anntzer force-pushed the minoroverstrike branch from d953c0b to a752d46 Compare February 16, 2019 20:35

anntzer removed the status: needs rebase label Feb 16, 2019

anntzer removed the status: needs rebase label Feb 17, 2019

jklymak merged commit 2658eef into matplotlib:master Feb 23, 2019

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 23, 2019

Backport PR matplotlib#13314: Move major/minor tick overstrike logic …

5591ce6

…to Axis.

meeseeksmachine mentioned this pull request Feb 23, 2019

Backport PR #13314 on branch v3.1.x (Move major/minor tick overstrike logic to Axis.) #13498

Merged

timhoffm added a commit that referenced this pull request Feb 23, 2019

Merge pull request #13498 from meeseeksmachine/auto-backport-of-pr-13…

f4a699b

…314-on-v3.1.x Backport PR #13314 on branch v3.1.x (Move major/minor tick overstrike logic to Axis.)

anntzer deleted the minoroverstrike branch February 23, 2019 21:11

anntzer mentioned this pull request Mar 7, 2019

Minor ticklabels are missing at positions of major ticks. #13618

Closed

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 1, 2019

FIX: restore de-confliction logic for minor ticks

7680c19

In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 9, 2019

FIX: restore de-confliction logic for minor ticks

18b2bcc

In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 12, 2019

FIX: restore de-confliction logic for minor ticks

8d4cee5

In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move major/minor tick overstrike logic to Axis. #13314

Move major/minor tick overstrike logic to Axis. #13314

anntzer commented Jan 29, 2019 •

edited

Loading

anntzer commented Jan 29, 2019 •

edited

Loading

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019

anntzer commented Jan 29, 2019

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019 •

edited

Loading

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 30, 2019 •

edited

Loading

jklymak commented Jan 30, 2019

anntzer commented Jan 30, 2019 •

edited

Loading

jklymak left a comment

jklymak Jan 30, 2019

anntzer Jan 30, 2019

jklymak Jan 30, 2019

anntzer Jan 30, 2019

jklymak Feb 1, 2019

anntzer Feb 1, 2019 •

edited

Loading

anntzer commented Feb 12, 2019 •

edited

Loading

timhoffm Feb 16, 2019

anntzer Feb 16, 2019

anntzer commented Feb 16, 2019

timhoffm commented Feb 16, 2019

tacaswell commented Feb 23, 2019

Move major/minor tick overstrike logic to Axis. #13314

Move major/minor tick overstrike logic to Axis. #13314

Conversation

anntzer commented Jan 29, 2019 • edited Loading

PR Summary

PR Checklist

anntzer commented Jan 29, 2019 • edited Loading

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019

anntzer commented Jan 29, 2019

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 29, 2019 • edited Loading

anntzer commented Jan 29, 2019

jklymak commented Jan 29, 2019

anntzer commented Jan 30, 2019 • edited Loading

jklymak commented Jan 30, 2019

anntzer commented Jan 30, 2019 • edited Loading

jklymak left a comment

Choose a reason for hiding this comment

jklymak Jan 30, 2019

Choose a reason for hiding this comment

anntzer Jan 30, 2019

Choose a reason for hiding this comment

jklymak Jan 30, 2019

Choose a reason for hiding this comment

anntzer Jan 30, 2019

Choose a reason for hiding this comment

jklymak Feb 1, 2019

Choose a reason for hiding this comment

anntzer Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

anntzer commented Feb 12, 2019 • edited Loading

timhoffm Feb 16, 2019

Choose a reason for hiding this comment

anntzer Feb 16, 2019

Choose a reason for hiding this comment

anntzer commented Feb 16, 2019

timhoffm commented Feb 16, 2019

tacaswell commented Feb 23, 2019

anntzer commented Jan 29, 2019 •

edited

Loading

anntzer commented Jan 29, 2019 •

edited

Loading

anntzer commented Jan 29, 2019 •

edited

Loading

anntzer commented Jan 30, 2019 •

edited

Loading

anntzer commented Jan 30, 2019 •

edited

Loading

anntzer Feb 1, 2019 •

edited

Loading

anntzer commented Feb 12, 2019 •

edited

Loading