Better choice of offset-text. #5785

anntzer · 2016-01-02T08:52:18Z

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.

WeatherGod · 2016-01-06T18:09:34Z

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

anntzer · 2016-01-06T20:55:52Z

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

anntzer · 2016-01-06T21:17:35Z

(hum, the failing test seems to work for me (and doesn't involve offset texts))

tacaswell · 2016-01-07T01:51:17Z

lib/matplotlib/tests/test_ticker.py

@@ -159,6 +159,49 @@ def test_SymmetricalLogLocator_set_params():
    nose.tools.assert_equal(sym.numticks, 8)


+def test_ScalarFormatter_offset_value():


This needs an @cleanup decorator

As it is, the @cleanup decorator doesn't support generative tests. I'll write an patch for that first.

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.

WeatherGod · 2016-01-27T16:50:54Z

Huh, never seen this error before: "RuntimeError: In set_text: could not load glyph". Also, this branch needs rebasing.

anntzer · 2016-01-27T20:04:43Z

Rebased. I cannot reproduce the glyph-loading errors locally.

QuLogic · 2016-02-18T01:29:02Z

lib/matplotlib/ticker.py

+        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:


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

VincentVandalon · 2016-03-21T08:12:06Z

lib/matplotlib/ticker.py

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


I fully agree that renaming this method was really needed; the new name actually reflects the purpose of the function.

Contra my reluctance to change the public API at all, changing anything with a leading _ is fair game.

efiring · 2016-05-02T00:57:18Z

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

@WeatherGod

Tests cases courtesy of @WeatherGod. Slightly improved numerical accuracy.

anntzer · 2016-05-02T01:15:23Z

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?

efiring · 2016-05-02T03:08:55Z

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.

anntzer · 2016-05-02T04:14:24Z

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.

WeatherGod · 2016-05-02T14:01:58Z

This is looking good to me. Were there any remaining concerns? Do we need to update any documentation about the improved offset text algorithm?

anntzer · 2016-05-02T15:52:36Z

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?

efiring · 2016-05-02T17:09:30Z

@anntzer Yes, adding that explanation would be good.

anntzer · 2016-05-02T20:13:22Z

Done.

dopplershift · 2016-05-11T16:33:06Z

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 · 2016-05-11T16:42:18Z

I don't see a way to restart just one appveyor test, so I triggered the whole set of checks.

dopplershift · 2016-05-11T17:01:54Z

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 )

Better choice of offset-text.

efiring · 2016-05-11T18:26:46Z

Backported to v2.x as beb08c8.

tacaswell added the status: needs review label Jan 2, 2016

tacaswell added this to the next major release (2.0) milestone Jan 2, 2016

tacaswell reviewed Jan 7, 2016
View reviewed changes

anntzer force-pushed the better-offsettext-choice branch from 6c2089d to 313a998 Compare January 7, 2016 03:53

anntzer mentioned this pull request Jan 7, 2016

Support generative tests in @cleanup. #5809

Merged

anntzer force-pushed the better-offsettext-choice branch from 313a998 to 0a9df25 Compare January 27, 2016 20:00

QuLogic reviewed Feb 18, 2016
View reviewed changes

anntzer force-pushed the better-offsettext-choice branch from 0a9df25 to 39efb99 Compare February 18, 2016 08:27

tacaswell closed this Feb 18, 2016

tacaswell reopened this Feb 18, 2016

tacaswell added status: needs review and removed status: needs review labels Feb 18, 2016

anntzer force-pushed the better-offsettext-choice branch from 39efb99 to 80de215 Compare February 21, 2016 17:32

tacaswell mentioned this pull request Mar 21, 2016

Offset and scaling factors in axis format #4376 #6086

Closed

VincentVandalon reviewed Mar 21, 2016
View reviewed changes

QuLogic mentioned this pull request Mar 26, 2016

Make @cleanup *really* support generative tests. #6022

Merged

anntzer added 3 commits May 1, 2016 18:07

Only use offset if it saves >=2 sig. digits.

f37fd3d

Tests cases courtesy of @WeatherGod. Slightly improved numerical accuracy.

Slightly more efficient impl.; more tests.

0c39a78

Fix test values.

9d0acb2

anntzer force-pushed the better-offsettext-choice branch from 80de215 to 9d0acb2 Compare May 2, 2016 04:13

Add What's new entry.

782b8a1

efiring closed this May 11, 2016

efiring reopened this May 11, 2016

efiring added status: needs review and removed status: needs review labels May 11, 2016

efiring merged commit 4e1792b into matplotlib:master May 11, 2016

efiring removed the status: needs review label May 11, 2016

efiring added a commit that referenced this pull request May 11, 2016

Merge pull request #5785 from anntzer/better-offsettext-choice

beb08c8

Better choice of offset-text.

anntzer deleted the better-offsettext-choice branch May 11, 2016 18:30

anntzer mentioned this pull request May 11, 2016

Better choice of axes offset value #5755

Closed

anntzer mentioned this pull request Sep 13, 2016

set offset threshold to 4 #7104

Closed

petehuang mentioned this pull request Jan 10, 2017

Minor offsetText formatting improvement #4451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better choice of offset-text. #5785

Better choice of offset-text. #5785

anntzer commented Jan 2, 2016

WeatherGod commented Jan 6, 2016

anntzer commented Jan 6, 2016

anntzer commented Jan 6, 2016

tacaswell Jan 7, 2016

anntzer Jan 7, 2016

tacaswell Jan 7, 2016

anntzer Jan 7, 2016

WeatherGod commented Jan 27, 2016

anntzer commented Jan 27, 2016

QuLogic Feb 18, 2016

VincentVandalon Mar 21, 2016

tacaswell Mar 21, 2016

efiring commented May 2, 2016

anntzer commented May 2, 2016

efiring commented May 2, 2016

anntzer commented May 2, 2016

WeatherGod commented May 2, 2016

anntzer commented May 2, 2016

efiring commented May 2, 2016

anntzer commented May 2, 2016

dopplershift commented May 11, 2016

efiring commented May 11, 2016

dopplershift commented May 11, 2016 •

edited

Loading

efiring commented May 11, 2016

		@@ -159,6 +159,49 @@ def test_SymmetricalLogLocator_set_params():
		nose.tools.assert_equal(sym.numticks, 8)


		def test_ScalarFormatter_offset_value():

Better choice of offset-text. #5785

Better choice of offset-text. #5785

Conversation

anntzer commented Jan 2, 2016

WeatherGod commented Jan 6, 2016

anntzer commented Jan 6, 2016

anntzer commented Jan 6, 2016

tacaswell Jan 7, 2016

Choose a reason for hiding this comment

anntzer Jan 7, 2016

Choose a reason for hiding this comment

tacaswell Jan 7, 2016

Choose a reason for hiding this comment

anntzer Jan 7, 2016

Choose a reason for hiding this comment

WeatherGod commented Jan 27, 2016

anntzer commented Jan 27, 2016

QuLogic Feb 18, 2016

Choose a reason for hiding this comment

VincentVandalon Mar 21, 2016

Choose a reason for hiding this comment

tacaswell Mar 21, 2016

Choose a reason for hiding this comment

efiring commented May 2, 2016

anntzer commented May 2, 2016

efiring commented May 2, 2016

anntzer commented May 2, 2016

WeatherGod commented May 2, 2016

anntzer commented May 2, 2016

efiring commented May 2, 2016

anntzer commented May 2, 2016

dopplershift commented May 11, 2016

efiring commented May 11, 2016

dopplershift commented May 11, 2016 • edited Loading

efiring commented May 11, 2016

dopplershift commented May 11, 2016 •

edited

Loading