Skip to content

STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 #7365

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.

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 3 commits into from
Nov 8, 2016

Conversation

efiring
Copy link
Member

@efiring efiring commented Oct 31, 2016

Closes #7104.

@efiring efiring added this to the 2.0 (style change major release) milestone Oct 31, 2016
@story645
Copy link
Member

Test to verify please?

@tacaswell
Copy link
Member

All of the failed test verify that it worked ;)

@tacaswell tacaswell added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. and removed Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Oct 31, 2016
@story645
Copy link
Member

story645 commented Nov 2, 2016

Love that it's now an RCparam and looks good to me.

@tacaswell
Copy link
Member

This needs to be added to the table in whats_new.rst and maybe to the default changes page (or create an issue asking me to do it).

@@ -169,7 +169,7 @@ def test_SymmetricalLogLocator_set_params():
nose.tools.assert_equal(sym.numticks, 8)


@cleanup
@cleanup(style='classic')
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the default style already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes for image_comparison, but no for cleanup. It looks to me like that is a bug in cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

I am having dejavu about this and some gordian knot we failed to deal with like 6mo ago....

Copy link
Member Author

Choose a reason for hiding this comment

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

Fortunately, it doesn't seem to matter much. style='classic' is needed two places in test_ticker, and a few more in the specific backend testers. The tests are working as they are. I don't think it is worth spending any time on right now.

@efiring efiring changed the title STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 [MRG] STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 Nov 2, 2016
@story645 story645 changed the title [MRG] STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 [MRG+1] STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 Nov 2, 2016
======================================================

With the default of ``rcParams['axes.formatter.useoffset'] = True``,
an offset will be used when it will save 4 or more digits. This can
Copy link
Member

Choose a reason for hiding this comment

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

unclear what you mean by save...maybe "will be used when the tick values have 4 or more digits"

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it is hard to figure out how to describe this, but it is not a matter of the number of digits the tick values have, it is a matter of how many fewer digits they can have if the offset is used. That's what is meant by "saved". I am using the original terminology of @anntzer from his comment in the code, I believe--though maybe that comment predates his refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe then use retain instead? Same basic meaning, but doesn't have associations with saving a figure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I introduced that terminology (see last chunk of https://github.com/matplotlib/matplotlib/pull/5785/files). It's not "retain", it's the leading digits that are not retained thanks to the use of the offset (i.e., exactly what @efiring said).

Copy link
Member

Choose a reason for hiding this comment

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

I'm so thoroughly confused since save is usually synonymous with retain and yet it's apparently not in this case and I'm not quite sure what it is a matter of how many fewer digits they can have if the offset is used. means in a practical sense.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant an example of what setting the offset does to the plot. Like I went back to the original issue and the offset code changed [1999-2000] changed the ticks to -10-10, but I don't know what the generic form of that is...basically, I'm also wondering if it makes sense to explain this functionality in terms of what it saves.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry. After writing out a description (as best as I could understand it), I'm not sure whether it will help clarify the docs, but maybe I'm just thinking along the wrong lines.

Let's just say there are two ticks, which can be written as abcd and efgh (where each of those letters is a single 0-9 digit). Then pick some other value between them jk00 so that the ticks are abcd - jk00 => -nopq and efgh - jk00 => rstu. If you can pick jk00 such that n == o == r == s == 0, then the offset is used.

The new setting means the "other value" must be jklm and additional conditions of p == q == t == u == 0 must also be satisfied (which it wouldn't be possible in this case, thus fixing dates.)

Copy link
Member

Choose a reason for hiding this comment

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

No worrys, as it sounds complicated. Is there a typical use case for this sort of offsetting or a name for it? (Like it's not scientific notation, which is also partly why it's so confusing)

It seems like it encodes ticks to reduce their size by offset or more digits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Example: an axis is showing years from 1990 through 2010. This could be shown with ticks from -10 to 10 with an offset of 2000. That is classic mode: 2 digits can be chopped off of the tick labels, so the offset is used if useoffset is True. With the new setting, saving those two digits in the tick labels is not enough, so the offset won't be used, and users will be happier. But if you have an axis from 20001 to 20003, then it will be given an offset of 20000, and the ticks will go from 1 to 3. The tick labels have been shortened by 4 digits, so they will have a better chance of fitting in the frame without overlapping.

A version of this was put in place long ago. Its purpose is to allow mpl to handle a wider range of cases by default, without excessively long tick labels.

Copy link
Member

@story645 story645 Nov 3, 2016

Choose a reason for hiding this comment

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

While that explanation is too long in docs, it's by far the clearest. Thanks!

So I think it's something like "Computes offset when ticks have more significant digits than offset_threshold. Displays tick-offset at each tick location to reduce tick label size to offset_threshold."

@anntzer
Copy link
Contributor

anntzer commented Nov 2, 2016

Perhaps the new rcparam can be folded into the already existing useoffset? Say 0 = no offset, higher = as this PR proposes?

@efiring
Copy link
Member Author

efiring commented Nov 3, 2016

@anntzer, I considered adding that functionality to useoffset. It would have been the way to go if we had a fresh start. Changing the meaning of an existing rcParam (or arg or kwarg) in that way, however, tends to be more trouble than it is worth, given the need for back-compatibility for a long time.

@anntzer
Copy link
Contributor

anntzer commented Nov 3, 2016

@efiring I was hoping that the rcfile parser could accept axes.formatter.useoffset : <number> and treat 0 as false and anything else as true (thus maintaining backcompatibility), but apparently that's not the case (specifically only 0 and 1 are supported) so I agree it's probably too hard to do this while maintaining backcompatibility.

I'll console myself by imagining that some day we'll deprecate the useoffset rcparam (folding it into offset_threshold) :-) (yes, I know, it's not going to happen).

@NelleV
Copy link
Member

NelleV commented Nov 7, 2016

Can we merge this? It is one of the release critical bug left for 2.0

@tacaswell tacaswell merged commit 6beddec into matplotlib:v2.x Nov 8, 2016
@QuLogic QuLogic changed the title [MRG+1] STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants