FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: minor log ticks overwrite by jklymak · Pull Request #13126 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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
anntzer merged 4 commits into from
Jan 11, 2019
Conversation 22 Commits 4 Checks 0 Files changed
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 .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
4 changes: 4 additions & 0 deletions lib/matplotlib/tests/test_ticker.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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))]

timhoffm Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

jklymak Jan 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

timhoffm Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jklymak Jan 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure thing
return ticklocs
return self.raise_if_exceeds(ticklocs)

def view_limits(self, vmin, vmax):
Expand Down
Back | FazBrowse Home | New Git URL