Better choice of offset-text. #5785
Conversation
|
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)) |
| @@ -159,6 +159,49 @@ def test_SymmetricalLogLocator_set_params(): | |||
| nose.tools.assert_equal(sym.numticks, 8) | |||
|
|
|||
|
|
|||
| def test_ScalarFormatter_offset_value(): | |||
There was a problem hiding this comment.
This needs an @cleanup decorator
There was a problem hiding this comment.
As it is, the @cleanup decorator doesn't support generative tests. I'll write an patch for that first.
There was a problem hiding this comment.
Awesome. That is something that has driven me crazy a couple of times. I have dealt with it by putting the decorator on the called function and creating all of the figures/axes in that function.
6c2089d to
313a998
Compare
|
Huh, never seen this error before: "RuntimeError: In set_text: could not load glyph". Also, this branch needs rebasing. |
313a998 to
0a9df25
Compare
|
Rebased. I cannot reproduce the glyph-loading errors locally. |
| 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: |
There was a problem hiding this comment.
Can move this section above the absolute values to short-circuit a little earlier.
0a9df25 to
39efb99
Compare
39efb99 to
80de215
Compare
| 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): |
There was a problem hiding this comment.
I fully agree that renaming this method was really needed; the new name actually reflects the purpose of the function.
There was a problem hiding this comment.
Contra my reluctance to change the public API at all, changing anything with a leading _ is fair game.
|
This is failing the two tests in which |
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 matplotlib#5780) 99990.5, 100000.5 => 100000 1233999, 1234001 => 1234000 (and the same for negative limits). See matplotlib#5755.
Tests cases courtesy of @WeatherGod. Slightly improved numerical accuracy.
|
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 |
80de215 to
9d0acb2
Compare
|
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? |
|
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 ) |
|
Backported to v2.x as beb08c8. |
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.