-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
Test to verify please? |
All of the failed test verify that it worked ;) |
Love that it's now an RCparam and looks good to me. |
This needs to be added to the table in |
@@ -169,7 +169,7 @@ def test_SymmetricalLogLocator_set_params(): | |||
nose.tools.assert_equal(sym.numticks, 8) | |||
|
|||
|
|||
@cleanup | |||
@cleanup(style='classic') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
====================================================== | ||
|
||
With the default of ``rcParams['axes.formatter.useoffset'] = True``, | ||
an offset will be used when it will save 4 or more digits. This can |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
Perhaps the new rcparam can be folded into the already existing |
@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. |
@efiring I was hoping that the rcfile parser could accept I'll console myself by imagining that some day we'll deprecate the |
Can we merge this? It is one of the release critical bug left for 2.0 |
Closes #7104.