GeezerVison

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

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.

This is failing the two tests in which left==right. It's not clear to me whether the problem is in the algorithm, or in the tests.

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

The present algorithm is giving (1, 1, 1) and (123, 123, 120) as left, right, offset. Nonsingular is expanding the actual left, right ranges to (0.999, 1.001) and (122.877, 123.123). So the test cases with left==right are not really special cases. Either we like what the algorithm does, or we don't. I still don't completely understand what the algorithm's criteria are, but the results at least look reasonable, so I would be inclined to just change the tests to match. Maybe the tests should actually be changed to use the values provided by the present implementation of nonsingular rather than feeding in equal numbers, since otherwise a change to nonsingular would cause this test to fail for reasons not related to the algorithm it is testing.

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.