FIX: minor log ticks overwrite #13126

jklymak · 2019-01-07T04:52:50Z

PR Summary

needs tests...

attempting to fix log overprint issue (closes #13112)

import matplotlib.pyplot as plt

fig, axs = plt.subplots(3,3, figsize=(8, 8), constrained_layout=True)

axs[0,0].semilogy([10000, 2500], [10000, 10010])
axs[0,1].semilogy([10000, 2500], [10000, 10100])
axs[0,2].semilogy([10000, 2500], [10000, 11000])

axs[1,0].semilogy([10000, 2500], [10000, 25000])
axs[1,1].semilogy([10000, 2500], [10000, 55000])
axs[1,2].semilogy([10000, 2500], [10000, 85000])

axs[2,0].semilogy([10000, 2500], [10000, 100000])
axs[2,1].semilogy([10000, 2500], [10000, 120000])
axs[2,2].semilogy([10000, 2500], [10000, 850000])

#ax.set_yticks([10000, 20000, 25000])
plt.show()

Before:

After

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-07T15:17:40Z

I think the change on ticker.py's side should look something like

diff --git i/lib/matplotlib/ticker.py w/lib/matplotlib/ticker.py
index 89e077a04..84bd9a2b4 100644
--- i/lib/matplotlib/ticker.py
+++ w/lib/matplotlib/ticker.py
@@ -2093,9 +2093,7 @@ def is_decade(x, base=10):
 
 
 def is_close_to_int(x):
-    if not np.isfinite(x):
-        return False
-    return abs(x - round(x)) < 1e-10
+    return abs(x - np.round(x)) < 1e-10
 
 
 class LogLocator(Locator):
@@ -2256,7 +2254,11 @@ class LogLocator(Locator):
             # If we're a minor locator *that expects at least two ticks per
             # decade* and the major locator stride is 1 and there's no more
             # than one minor tick, switch to AutoLocator.
-            return AutoLocator().tick_values(vmin, vmax)
+            ticklocs = AutoLocator().tick_values(vmin, vmax)
+            # Don't overstrike the major labels.
+            ticklocs = ticklocs[
+                ~is_close_to_int(np.log(ticklocs) / np.log(b))]
+            return ticklocs
         return self.raise_if_exceeds(ticklocs)
 
     def view_limits(self, vmin, vmax):

after which I don't know if the other changes (touching labelOnlyBase) are needed?

jklymak · 2019-01-07T15:23:26Z

Yeah that might be better.

Not sure we should be implying that a locator is a minor locator. My preference would be that we just explicitly pass the major locator instance to anything we want to be a minor locator and then do a set diff between the two locators Or simply doing this in the axis code after the fact where we know about both locators.

anntzer · 2019-01-07T15:28:26Z

The log locator code already has "magic" to guess whether it's a minor or a major locator; given that it's already assuming that it works we may as well rely on it.
As argued in a bunch of other places it is indeed suboptimal that tickers don't really know whether they are major or minor, but adding more logic to clean that up on the axis side seems worse.

jklymak · 2019-01-07T17:03:16Z

I'd argue that the "magic" is probably a bad idea because it relies on the user knowing the magic when they specify the Locator, and as we have seen, even the developers don't get the "magic" right.

I think there are two approaches - 1) tell the locator explicitly what ticks to avoid with an optional major_ticks=None kwarg, or 2) get the ticks from both locators, and then exclude any overlaps in the minor ticks.

I guess I think 1) is the best because I can think of cases where the minorLocator might want to exclude a tick because it is close but not equal to a major tick. I don't see why we can't impliment this as a Locator._remove_duplicates method that defaults on all Locators to removing all the duplicates in the __call__. If child classes want to ignore all this they can, so I don't think it'd be a backwards incompatibility issue.

jklymak · 2019-01-07T17:41:04Z

(BTW implimenting your change to squash the bug, the other discussion is more long-term)

anntzer · 2019-01-07T18:13:21Z

lib/matplotlib/ticker.py

-            return AutoLocator().tick_values(vmin, vmax)
+            ticklocs = AutoLocator().tick_values(vmin, vmax)
+            # Don't overstrike the major labels.
+            ticklocs = [t for t in ticklocs if 


eh, that needs to use boolean indexing, no?

Not sure what you mean? Your version didn't work because ticklocs is an array and is_close_to_int only works on a single value. This seems to work...

My patch also fixed is_close_to_int to be vectorized: it deleted the lines

- if not np.isfinite(x): - return False

you may have missed that?
(if x is not finite, the expression below will also be false).

Ah, my fault.

timhoffm · 2019-01-07T19:23:07Z

I think there are two approaches - 1) tell the locator explicitly what ticks to avoid with an optional major_ticks=None kwarg, or 2) get the ticks from both locators, and then exclude any overlaps in the minor ticks.

I would name this exclude_ticks=None, which is more general. It may take a list of values and/or (possibly also later implemented) a locator.

anntzer · 2019-01-09T23:47:13Z

lib/matplotlib/ticker.py

+            # Don't overstrike the major labels.
+            ticklocs = ticklocs[
+                ~is_close_to_int(np.log(ticklocs) / np.log(b))]
+            print('ticklocs', ticklocs)


remove this line

anntzer · 2019-01-09T23:47:23Z

lib/matplotlib/ticker.py

@@ -2250,13 +2248,19 @@ def tick_values(self, vmin, vmax):
                ticklocs = b ** decades

        _log.debug('ticklocs %r', ticklocs)
+        print('subs', subs, have_subs, ticklocs)


remove this line

Oooops, sorry

anntzer

I guess that can count as Jody's approval of my patch :)

timhoffm · 2019-01-10T08:31:51Z

lib/matplotlib/ticker.py

+            ticklocs = AutoLocator().tick_values(vmin, vmax)
+            # Don't overstrike the major labels.
+            ticklocs = ticklocs[
+                ~is_close_to_int(np.log(ticklocs) / np.log(b))]


This looks like a hard-coded assumption on the position of the major ticks. Will this fail if I’ve set non-default values no the major locator?

Absolutely. See discussion above for fixing that. But that assumption is already in the code, this just makes that assumption work properly under all conditions. The next step is to get rid of the assumption, but thats a bigger project.

Ok, that's reasonable for now. However, it would be good to explicitly document this in the code with a comment (unless you're immediately following up with the full fix).

timhoffm

Anybody can merge after CI pass.

lumberbot-app · 2019-01-11T00:25:12Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.0.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 71dd3ae125d060bc08193abb93cca643245e3de0

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #13126: FIX: minor log ticks overwrite'

Push to a named branch :

git push YOURFORK v3.0.x:auto-backport-of-pr-13126-on-v3.0.x

Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR #13126 on branch v3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

timhoffm · 2019-01-12T18:17:11Z

Automatic backport fails due to #12865 not being backported. We should either backport both or none.

tacaswell · 2019-01-12T20:43:46Z

Moved both to 3.1. The logic around the log ticks is subtle and probably better to keep changes to it for minor release (not patch releases).

jklymak force-pushed the fix-minor-log branch from 7cf32f3 to fc3bc64 Compare January 7, 2019 17:40

anntzer reviewed Jan 7, 2019

View reviewed changes

FIX: log axes: don't choose major ticks as a minor tick

339d762

jklymak force-pushed the fix-minor-log branch from fc3bc64 to 339d762 Compare January 7, 2019 18:24

jklymak changed the title ~~WIP/FIX: minor log ticks overwrite~~ FIX: minor log ticks overwrite Jan 7, 2019

jklymak added status: work in progress and removed status: work in progress labels Jan 7, 2019

jklymak force-pushed the fix-minor-log branch 2 times, most recently from 6e15b71 to 5c9d3bd Compare January 9, 2019 18:57

jklymak added this to the v3.0.3 milestone Jan 9, 2019

jklymak added the status: needs review label Jan 9, 2019

anntzer reviewed Jan 9, 2019

View reviewed changes

TST: add test

5e60314

jklymak force-pushed the fix-minor-log branch from 5c9d3bd to 5e60314 Compare January 9, 2019 23:56

anntzer approved these changes Jan 9, 2019

View reviewed changes

timhoffm reviewed Jan 10, 2019

View reviewed changes

DOC: slightly better commenting

419f6d3

timhoffm approved these changes Jan 10, 2019

View reviewed changes

Strike trailing white space

1f70d7a

anntzer merged commit 71dd3ae into matplotlib:master Jan 11, 2019

lumberbot-app bot added the status: needs manual backport label Jan 11, 2019

QuLogic removed the status: needs review label Jan 11, 2019

tacaswell modified the milestones: v3.0.3, v3.1 Jan 12, 2019

jklymak deleted the fix-minor-log branch January 12, 2019 21:00

timhoffm removed the status: needs manual backport label Jan 12, 2019

timhoffm mentioned this pull request Jan 28, 2019

Setting axis ticks in log scale produces duplicate tick labels. #11575

Closed

anntzer mentioned this pull request Jan 29, 2019

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

Merged

6 tasks

timhoffm mentioned this pull request Jul 14, 2020

ticklabels on log axes sometimes adds overwritten labels on top of existing ones #8622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: minor log ticks overwrite #13126

FIX: minor log ticks overwrite #13126

jklymak commented Jan 7, 2019 •

edited

Loading

anntzer commented Jan 7, 2019

jklymak commented Jan 7, 2019

anntzer commented Jan 7, 2019

jklymak commented Jan 7, 2019

jklymak commented Jan 7, 2019

anntzer Jan 7, 2019

jklymak Jan 7, 2019

anntzer Jan 7, 2019

jklymak Jan 7, 2019

timhoffm commented Jan 7, 2019

anntzer Jan 9, 2019

anntzer Jan 9, 2019

jklymak Jan 9, 2019

anntzer left a comment

timhoffm Jan 10, 2019

jklymak Jan 10, 2019

timhoffm Jan 10, 2019

jklymak Jan 10, 2019

timhoffm left a comment

lumberbot-app bot commented Jan 11, 2019

timhoffm commented Jan 12, 2019

tacaswell commented Jan 12, 2019

FIX: minor log ticks overwrite #13126

FIX: minor log ticks overwrite #13126

Conversation

jklymak commented Jan 7, 2019 • edited Loading

PR Summary

Before:

After

PR Checklist

anntzer commented Jan 7, 2019

jklymak commented Jan 7, 2019

anntzer commented Jan 7, 2019

jklymak commented Jan 7, 2019

jklymak commented Jan 7, 2019

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 commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer left a 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

Choose a reason for hiding this comment

lumberbot-app bot commented Jan 11, 2019

timhoffm commented Jan 12, 2019

tacaswell commented Jan 12, 2019

jklymak commented Jan 7, 2019 •

edited

Loading