Skip to content

Better choice of offset-text. #5785

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 5 commits into from
May 11, 2016
Merged

Better choice of offset-text. #5785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a4ecfb
Better choice of offset-text.
anntzer Jan 2, 2016
f37fd3d
Only use offset if it saves >=2 sig. digits.
anntzer Jan 6, 2016
0c39a78
Slightly more efficient impl.; more tests.
anntzer Feb 18, 2016
9d0acb2
Fix test values.
anntzer May 2, 2016
782b8a1
Add What's new entry.
anntzer May 2, 2016
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
Prev Previous commit
Next Next commit
Slightly more efficient impl.; more tests.
  • Loading branch information
@anntzer
anntzer committed May 2, 2016
commit 0c39a78b040e00caf87eee41d448a5e32c26cf54
3 changes: 3 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 @@ -180,6 +180,8 @@ def check_offset_for(left, right, offset):
(-100000.5, -99990.5, -100000),
(1233999, 1234001, 1234000),
(-1234001, -1233999, -1234000),
(1, 1, 0),
(123, 123, 123),
# Test cases courtesy of @WeatherGod
(.4538, .4578, .45),
(3789.12, 3783.1, 3780),
Expand All @@ -201,6 +203,7 @@ def check_offset_for(left, right, offset):

for left, right, offset in test_data:
yield check_offset_for, left, right, offset
Copy link
Member

Choose a reason for hiding this comment

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

I think all but two test cases are left < right; would it also make sense to yield in the reverse order? Also, I don't think there are any tests for left == right (though I don't see why that won't work correctly.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really think I need to support left == right because I don't see how this can ever happen. Other issues handled in new (rebased) commit.

Copy link
Member

Choose a reason for hiding this comment

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

Well, your code does check for the left == right case, so it's good so codify what the expected behaviour is in that case. I think the default locators would avoid this situation, but I'm not sure about user-created locators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll wait for #6022 to be merged in so that the tests are actuall run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

yield check_offset_for, right, left, offset


def _logfe_helper(formatter, base, locs, i, expected_result):
Expand Down
26 changes: 10 additions & 16 deletions lib/matplotlib/ticker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@
from matplotlib.externals import six

import decimal
import itertools
import locale
import math
import numpy as np
Expand Down Expand Up @@ -680,36 +681,29 @@ def _compute_offset(self):
self.offset = 0
return
lmin, lmax = locs.min(), locs.max()
# min, max comparing absolute values (we want division to round towards
# zero so we work on absolute values).
abs_min, abs_max = sorted([abs(float(lmin)), abs(float(lmax))])
# Only use offset if there are at least two ticks and every tick has
# the same sign.
if lmin == lmax or lmin <= 0 <= lmax:
Copy link
Member

Choose a reason for hiding this comment

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

Can move this section above the absolute values to short-circuit a little earlier.

self.offset = 0
return
# min, max comparing absolute values (we want division to round towards
# zero so we work on absolute values).
abs_min, abs_max = sorted([abs(float(lmin)), abs(float(lmax))])
sign = math.copysign(1, lmin)
# What is the smallest power of ten such that abs_min and abs_max are
# equal up to that precision?
# Note: Internally using oom instead of 10 ** oom avoids some numerical
# accuracy issues.
oom = math.ceil(math.log10(abs_max))
while True:
if abs_min // 10 ** oom != abs_max // 10 ** oom:
oom += 1
break
oom -= 1
oom_max = math.ceil(math.log10(abs_max))
oom = 1 + next(oom for oom in itertools.count(oom_max, -1)
if abs_min // 10 ** oom != abs_max // 10 ** oom)
if (abs_max - abs_min) / 10 ** oom <= 1e-2:
# Handle the case of straddling a multiple of a large power of ten
# (relative to the span).
# What is the smallest power of ten such that abs_min and abs_max
# at most 1 apart?
oom = math.ceil(math.log10(abs_max))
while True:
if abs_max // 10 ** oom - abs_min // 10 ** oom > 1:
oom += 1
break
oom -= 1
# are no more than 1 apart at that precision?
oom = 1 + next(oom for oom in itertools.count(oom_max, -1)
if abs_max // 10 ** oom - abs_min // 10 ** oom > 1)
# Only use offset if it saves at least two significant digits.
self.offset = (sign * (abs_max // 10 ** oom) * 10 ** oom
if abs_max // 10 ** oom >= 10
Expand Down