FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

STY: in ScalarFormatter, use offset to save at least 4 digits, not 2 by efiring · Pull Request #7365 · matplotlib/matplotlib · GitHub

Repository navigation

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

Merged
tacaswell merged 3 commits into
matplotlib:v2.xfrom
efiring:offset_threshold_4
Nov 8, 2016
Merged

tacaswell merged 3 commits into
matplotlib:v2.xfrom
efiring:offset_threshold_4

Conversation

efiring commented Oct 31, 2016

Copy link
Copy Markdown
Member

Closes #7104.

efiring added this to the 2.0 (style change major release) milestone Oct 31, 2016

Copy link
Copy Markdown
Member

Test to verify please?

Copy link
Copy Markdown
Member

All of the failed test verify that it worked ;)

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 commented Nov 2, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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).



@cleanup
@cleanup(style='classic')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't this the default style already?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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 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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

story645 Nov 3, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 2, 2016

Copy link
Copy Markdown
Contributor

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

efiring commented Nov 3, 2016

Copy link
Copy Markdown
Member Author

@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 commented Nov 3, 2016

Copy link
Copy Markdown
Contributor

@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 commented Nov 7, 2016

Copy link
Copy Markdown
Member

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

tacaswell merged commit 6beddec into matplotlib:v2.x Nov 8, 2016
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL