Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Better choice of offset-text. #5785
Conversation
tacaswell
added the
needs_review
label
Jan 2, 2016
tacaswell
added this to the
next major release (2.0)
milestone
Jan 2, 2016
|
I have some test cases from a while back when I last tried to come up with a better offset text logic. These test cases really helped explore the possible edge cases in that attempt. You might find it useful (the test data, that is, not the code): https://gist.github.com/WeatherGod/272f4022bf7a8ca12ff4 |
|
Thanks @WeatherGod, I incorporated them in the test suite and slightly changed the algorithm. Now, the main condition of whether to use the offset is whether there are two common significant digits at the beginning of every label (allowing for negative labels as discussed above, I think that's the main questionable choice remaining). |
|
(hum, the failing test seems to work for me (and doesn't involve offset texts)) |
tacaswell
and 1 other
commented on an outdated diff
Jan 7, 2016
| @@ -159,6 +159,49 @@ def test_SymmetricalLogLocator_set_params(): | ||
| nose.tools.assert_equal(sym.numticks, 8) | ||
| +def test_ScalarFormatter_offset_value(): |
anntzer
Contributor
|
|
Huh, never seen this error before: "RuntimeError: In set_text: could not load glyph". Also, this branch needs rebasing. |
|
Rebased. I cannot reproduce the glyph-loading errors locally. |
QuLogic
commented on an outdated diff
Feb 18, 2016
| - if ave_loc < 0: | ||
| - self.offset = (math.ceil(np.max(locs) / p10) * p10) | ||
| - else: | ||
| - self.offset = (math.floor(np.min(locs) / p10) * p10) | ||
| - else: | ||
| - self.offset = 0 | ||
| + if not len(locs): | ||
| + 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: |
QuLogic
Member
|
QuLogic
commented on the diff
Feb 18, 2016
| + (12592.82, 12591.43, 12590), | ||
| + (9., 12., 0), | ||
| + (900., 1200., 0), | ||
| + (1900., 1200., 0), | ||
| + (0.99, 1.01, 1), | ||
| + (9.99, 10.01, 10), | ||
| + (99.99, 100.01, 100), | ||
| + (5.99, 6.01, 6), | ||
| + (15.99, 16.01, 16), | ||
| + (-0.452, 0.492, 0), | ||
| + (-0.492, 0.492, 0), | ||
| + (12331.4, 12350.5, 12300), | ||
| + (-12335.3, 12335.3, 0)] | ||
| + | ||
| + for left, right, offset in test_data: | ||
| + yield check_offset_for, left, right, offset |
QuLogic
Member
|
QuLogic
commented on an outdated diff
Feb 18, 2016
| + 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 | ||
| + 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? |
|
|
QuLogic
commented on an outdated diff
Feb 18, 2016
| + # 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 | ||
| + 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)) |
QuLogic
Member
|
tacaswell
closed this
Feb 18, 2016
tacaswell
reopened this
Feb 18, 2016
tacaswell
added needs_review and removed needs_review
labels
Feb 18, 2016
tacaswell
referenced
this pull request
Mar 21, 2016
Open
Offset and scaling factors in axis format #4376 #6086
VincentVandalon
commented on the diff
Mar 21, 2016
| self._set_orderOfMagnitude(d) | ||
| self._set_format(vmin, vmax) | ||
| - def _set_offset(self, range): | ||
| - # offset of 20,001 is 20,000, for example | ||
| + def _compute_offset(self): |
VincentVandalon
|
QuLogic
referenced
this pull request
Mar 26, 2016
Merged
Make @cleanup *really* support generative tests. #6022
|
This is failing the two tests in which |
|
The test failure was probably masked at some point by the misuse of a generative test. Anyways, this comes down to a policy choice for the left == right case. I guess that "by continuity", we should just let the offset be equal to the single value in that case (say the values were x-eps and x+eps, then it would be normal for x (rounded to ~eps) to be the offset, so we should keep that as eps->0). |
|
The present algorithm is giving |
|
OK, now I remember what I did: an offset text is used if it "saves" at least two significant digits, i.e. if the length of the (low, high) range is no more than a hundredth of the largest power of ten below high. So yes, the (1, 1, 1) and (123, 123, 120) outputs are expected given the effect of nonsingular. I rewrote the tests (still using equal left and right for now because using different left and right is basically already tested) and rebased on master. |
|
This is looking good to me. Were there any remaining concerns? Do we need to update any documentation about the improved offset text algorithm? |
|
I can add a snippet like "The default offset-text choice was changed to only use significant digits that are common to all ticks (e.g. 1231..1239 -> 1230, instead of 1231), except when they straddle a relatively large multiple of a power of ten in which case that multiple is chosen (e.g. 1999..2001->2000)." Looks good? |
|
@anntzer Yes, adding that explanation would be good. |
|
Done. |
|
Looks like the only failure on AppVeyor was spurious. Anyone with proper permissions wanna restart the appveyor run so this can go green and be merged? |
efiring
closed this
May 11, 2016
efiring
reopened this
May 11, 2016
efiring
added needs_review and removed needs_review
labels
May 11, 2016
|
I don't see a way to restart just one appveyor test, so I triggered the whole set of checks. |
|
If you have permissions, you can log into AppVeyor and re-run the build. Unfortunately, it doesn't key off of GitHub permissions; those permissions are managed manually by whoever owns the AppVeyor project (looks like @mdboom ) |
efiring
merged commit 4e1792b
into matplotlib:master
May 11, 2016
efiring
removed the
needs_review
label
May 11, 2016
efiring
added a commit
that referenced
this pull request
May 11, 2016
|
|
efiring |
beb08c8
|
|
Backported to v2.x as beb08c8. |
anntzer commentedJan 2, 2016
The axis offset text is chosen as follows:
xlims => offsettext
123, 189 => 0
12341, 12349 => 12340
99999.5, 100010.5 => 100000 # (also a test for #5780)
99990.5, 100000.5 => 100000
1233999, 1234001 => 1234000
(and the same for negative limits).
See #5755.