Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Proposed change to default log scale tick formatting #5161
Conversation
tacaswell
added the
needs_review
label
Oct 1, 2015
jenshnielsen
added the
default changes
label
Oct 1, 2015
jenshnielsen
added this to the
next major release (2.0)
milestone
Oct 1, 2015
|
It seems like you've combined the logic of whether to display a tick (which is usually the job of a Locator subclass) with the logic of how to display a tick (which is usually the job of a Formatter subclass). Any reason why you didn't separate concerns like all the other Locator/Formatters? Other than that, I see this as very useful, though we'll have to determine whether or not it's a good idea to change the default behavior. |
|
@mdboom: For the case of log-scaled axis, it is nice to show all minor ticks to make it clear that the scaling is logarithmic, but we obviously do not want to label all minor ticks. The current default is show all minor ticks and label none. I had to combine them so that all minor ticks are shown, but not all of them labeled. I understand its a departure from how other Locator/Formatter work, but I did not see a different way to do it. EDIT: I could not find it, but is there any other Locator/Formatter where this is done (i.e., only some of the shown minor or major ticks are labeled)? |
|
@zblz: I understand the problem better now. I think it could lead to confusion that the formatter chooses not to format some ticks, particularly if using an explicit locator or something. However, as you say, I can't think of a better way right now. That's all the more argument to keep the default for log scales as something that formats everything. Let me think on it for a bit -- maybe there is a better way, perhaps by adding the ability to have separate locators for ticks and text... |
|
Separate locators would be ideal for this case, but it would increase the complexity of the tick logic in By the way, I adapted this logic of not showing some labels from the only way I knew how to produce this previously, which was to use horrible, horrible ad-hoc labels like this:
|
|
What if a locator could signal whether the location is intended for On Tue, Oct 6, 2015 at 1:58 PM, Victor Zabalza notifications@github.com
|
|
@WeatherGod: I like that idea. |
|
@WeatherGod: It sounds good, but I have no idea of how to even begin to implement that from a glance at |
|
If one works from the assumption that all ticklabels must have a tickline, On Tue, Oct 6, 2015 at 2:15 PM, Victor Zabalza notifications@github.com
|
|
The |
|
@tacaswell: But if whether we return a label depends on |
|
Ok, I tried to move the print logic from the formatter to the locator following @WeatherGod's suggestion, and now the locator must return two arrays: the location for the ticks and whether each tick should be labeled. This means a change to all the |
tacaswell
modified the milestone: proposed next point release (2.1), next major release (2.0)
Oct 7, 2015
|
re-milestoned this for 2.1 as this is not just changing some parameters. |
|
Change of mind right after pushing: I moved the logic on whether to show the labels to an different function than the |
|
To keep the focus of this PR, I have removed the functionality to plot |
|
Thanks. I think the separation of Locator/Formatter is much better now. One further refinement though -- I don't think we necessarily require that a Locator inherits from LocatorBase (matplotlib's built-in ones all do, but third-party ones may not). So we should be robust to the possibility that |
|
Thanks. I'm happy to merge this once the test failure is resolved. |
|
|
|
We could consider the default change as part of 2.0. Not promising anything, but that's a good window of opportunity for it. |
|
I would like to suggest re-milestoning this (or the original issue, #4867), for 2.0. When round-number limits were the default, a log-scale axis would by default be expanded to initially cover at least one (integer) decade, and thus have at least two labeled ticks (at the two ends). With the switch to margins-based limits, it is now possible for axes to default to having no labels at all (e.g. Edit: Changed the milestone, but feel free to discuss if you disagree. |
anntzer
modified the milestone: 2.0 (style change major release), 2.1 (next point release)
Jun 27, 2016
|
power-cycling to trigger Travis on latest master... |
WeatherGod
closed this
Jul 16, 2016
mdboom
removed the
needs_review
label
Jul 16, 2016
WeatherGod
reopened this
Jul 16, 2016
mdboom
added the
needs_review
label
Jul 16, 2016
efiring
and 1 other
commented on an outdated diff
Jul 20, 2016
| @@ -1117,7 +1117,7 @@ def _get_font(self, font): | ||
| cached_font = self.fonts.get(basename) | ||
| if cached_font is None: | ||
| fname = os.path.join(self.basepath, basename + ".afm") | ||
| - with open(fname, 'r') as fd: | ||
| + with open(fname, 'rb') as fd: |
zblz
Member
|
|
@zblz Would you be able to add a test + an example? |
anntzer
added a commit
to anntzer/matplotlib
that referenced
this pull request
Jul 24, 2016
|
|
zblz + anntzer |
cdfdee9
|
|
I am working on a test and example. In the meantime, what is the consensus on whether to make this a default or not? In addition, the PR in #4730 has not been developed further, but I think this PR and that one complement each other nicely in that having several labels that look like |
|
Tests added, example still missing. |
|
I think it would be better to keep #4730 as a separate PR; you could take it over if @astrofrog doesn't have the time or inclination to finish it. |
zblz
referenced
this pull request
Jul 25, 2016
Closed
[WIP] Proposed improvement in default log formatting #4730
jenshnielsen
commented on an outdated diff
Aug 1, 2016
| @@ -63,6 +72,31 @@ def test_LogLocator(): | ||
| test_value = np.array([0.5, 1., 2., 4., 8., 16., 32., 64., 128., 256.]) | ||
| assert_almost_equal(loc.tick_values(1, 100), test_value) | ||
| + # test label locator |
jenshnielsen
Owner
|
|
Can you please move the logic if a tick label or an empty string should be emited into the formatter logic (and not add new public API). If there is a performance bottle neck, we should try to deal with that via private caching/memorized private methods. |
tacaswell
added needs_revision and removed needs_review
labels
Aug 1, 2016
I agree that it would be better to keep this sort of logic private, but @mdboom argued some time ago that a We could also keep it in |
|
Locators place ticks, and a formatter either labels all of them, or none of them ( My conclusion is that the decision as to which tick locations to label is properly the function of the formatter, and it should be done via the |
|
Nice writeup of the behavior of the formatter class. I feel like this suggests that the API for the Formatter should be rethought. Fundamentally, there should just be one main method (let's call it I said one main method because there's actually a second method needed: formatting for the cursor (currently |
|
@efiring: Thanks for the perspective on the formatter classes, very useful! Considering this, I'll move the logic back into the Fomatter, with as much of it as possible in |
zblz
added some commits
Oct 1, 2015
|
I have moved the logic back into the formatter classes, choosing to add it into I have also added back |
|
Getting a bunch of errors. Somehow, zeros are getting down into |
|
I have fixed the error on symlog axes, which where sending values |
zblz
referenced
this pull request
Aug 3, 2016
Merged
Fix read mode when loading cached AFM fonts #6898
|
The remaining error on |
|
power-cycling to test against the latest master. |
WeatherGod
closed this
Aug 4, 2016
mdboom
removed the
needs_revision
label
Aug 4, 2016
WeatherGod
reopened this
Aug 4, 2016
mdboom
added the
needs_review
label
Aug 4, 2016
|
This is passing and is milestoned for v2.0. I don't know who is the one to approve the remaining style changes. |
tacaswell
merged commit 947e6eb
into matplotlib:master
Aug 22, 2016
tacaswell
removed the
needs_review
label
Aug 22, 2016
tacaswell
added a commit
that referenced
this pull request
Aug 22, 2016
|
|
tacaswell |
fb45c4a
|
|
backported to v2.x as fb45c4a |
tacaswell
added a commit
to tacaswell/matplotlib
that referenced
this pull request
Aug 29, 2016
|
|
tacaswell |
3a03595
|
This was referenced Aug 29, 2016
efiring
added a commit
to efiring/matplotlib
that referenced
this pull request
Nov 7, 2016
|
|
efiring |
129a32b
|
zblz commentedOct 1, 2015
Following #4730 and #4867, this is a proposal to change the defaults for the upcoming 2.0 release regarding the default tick label formatting for log-scaled axes. This is not a finished PR, but mean to initiate discussion on whether this is desired as default (if not I still think a similar thing could be worth having as an option).
LogFormatterSciNotation) is used as default for all log-scaled axes. This formatter always prints all labels for the bases of the log scale (major ticks) and adds labels to the minor ticks as the axis view limits are made smaller (with a similar logic toLogLocatorwithsubs=None):(1, 3) * base**iare labeled.(1, 2, 5) * base**iare labeled.(1, 2, 4, 7) * base**iare labeled.$ 3 \times 10^{-2} $LogFormatter).Example of this formatter in action:

The main motivation is to improve readability of the axis ticks when log-scaled axis are zoomed-in to less than a few decades, which under the current Formatters leaves only very few labels in the axis. Note that this does not change the formatting whenever more than 3 decades are spanned by the axis view limits.