Skip to content

FIX: minor log ticks overwrite #13126

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.

Sign up for GitHub

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 4 commits into from
Jan 11, 2019
Merged

FIX: minor log ticks overwrite #13126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
339d762
FIX: log axes: don't choose major ticks as a minor tick
jklymak Jan 7, 2019
5e60314
TST: add test
jklymak Jan 9, 2019
419f6d3
DOC: slightly better commenting
jklymak Jan 10, 2019
1f70d7a
Strike trailing white space
jklymak Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/matplotlib/tests/test_ticker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ def test_switch_to_autolocator(self):
loc = mticker.LogLocator(subs="all")
assert_array_equal(loc.tick_values(0.45, 0.55),
[0.44, 0.46, 0.48, 0.5, 0.52, 0.54, 0.56])
# check that we *skip* 1.0, and 10, because this is a minor locator
loc = mticker.LogLocator(subs=np.arange(2, 10))
assert 1.0 not in loc.tick_values(0.9, 20.)
assert 10.0 not in loc.tick_values(0.9, 20.)

def test_set_params(self):
"""
Expand Down
11 changes: 7 additions & 4 deletions lib/matplotlib/ticker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -2256,7 +2254,12 @@ def tick_values(self, vmin, vmax):
# 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. Assumes major locs are
# at b = self._base
ticklocs = ticklocs[
~is_close_to_int(np.log(ticklocs) / np.log(b))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing

return ticklocs
return self.raise_if_exceeds(ticklocs)

def view_limits(self, vmin, vmax):
Expand Down