Skip to content
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

ENH: restore default ability to label some minor log ticks. #7419

Merged
merged 1 commit into from Nov 8, 2016

Conversation

efiring
Copy link
Member

@efiring efiring commented Nov 7, 2016

This partly restores the functionality that was added in
PR #5161 and partly removed in #7000. The "partly" is because
now the labeling of minor log ticks is turned on only when
numdecs (the axis range in powers of the log base) is less
than or equal to one, rather than 3.

This also fixes a bug that was causing double labeling with a
base of 2; minor ticks were coinciding with major ticks, and
both were being labeled.

This partly restores the functionality that was added in
PR matplotlib#5161 and partly removed in matplotlib#7000.  The "partly" is because
now the labeling of minor log ticks is turned on only when
numdecs (the axis range in powers of the log base) is less
than or equal to one, rather than 3.

This also fixes a bug that was causing double labeling with a
base of 2; minor ticks were coinciding with major ticks, and
both were being labeled.
@efiring efiring added this to the 2.0 (style change major release) milestone Nov 7, 2016
subs = np.array([1.0])
elif numdec > 6:
if self._subs is None: # autosub for minor ticks
if numdec > 10 or b < 3:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should add a comment why no minor ticks should be label when b > 3.

@@ -853,7 +853,7 @@ def set_locs(self, locs):
vmax = math.log(vmax) / math.log(b)
numdec = abs(vmax - vmin)

if numdec > 3:
if numdec > 1:
Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents is that we are trying to be too smart here and it has unintended consequences for anyone trying to use this formatter. At most, this sublabel filtering should be disabled by default (and possibly activated internally).

Also, the sublabel should probably (if kept) be private.

@NelleV
Copy link
Member

NelleV commented Nov 7, 2016

Overall, I think restoring the minor tick labels when one decode or less is displayed is a good idea.
On the other hand, the sublabeling filtering seems not only to have unintended and difficult to debug consequences.
If the minor ticks are displayed only when there is one or less decade on the plot, that means there are at most 9 labels to display. IMO, it renders the sublabeling filtering unecessary (hence #7414 to remove it).

@efiring
Copy link
Member Author

efiring commented Nov 7, 2016

@NelleV, without sublabel filtering in some form, the sublabels will often be too crowded on any subplot, especially for the last few--7, 8, and 9. I think the filtering could be improved, though, so that the all ticks would be labeled when there are very few of them. With a log scale, very few labels are needed to orient the viewer, and excess labels rapidly become clutter even if they don't overlap--which they are prone to do.

@NelleV NelleV changed the title ENH: restore default ability to label some minor log ticks. [MRG+1] ENH: restore default ability to label some minor log ticks. Nov 7, 2016
@efiring
Copy link
Member Author

efiring commented Nov 7, 2016

Note from Monday AM discussion: this can be merged, and then followed up with additional changes in #7414. A new kwarg is needed to control the subsetting of label formatting.

@NelleV
Copy link
Member

NelleV commented Nov 7, 2016

@tacaswell Do you want to check this PR out?

@tacaswell
Copy link
Member

tacaswell commented Nov 8, 2016

I understand @NelleV 's point about this being a bit too smart and typically would agree, however in this case I think this is the least-bad option.

This is mostly a re-hash of the discussion last week, but typing this out for the record and to make sure I really agree with it ;)

The locators decide where to put ticks and the formatters decide how to label them. For most linear axis situations, you label all major ticks an no minor ticks and the default auto-locator keeps there being a decent number of major ticks visible.

For log we are further constrained that decadal (base ** N) ticks are major, and intra-decadal ticks (m*base ** N) ticks are minor (by convention) and the same labeling rules apply. The down side of this is that you can end up with no decadal ticks in the view range and hence, no tick labels. To avoid this we either need to change the major locator to switch from decadal to intra-decadal ticks (which breaks a pretty strong convention) or let the formatter on the minor ticks sometimes provide a non-empty string depending on the view limits.

A more general way to fix this would be to add a ShouldLabelThisTickDecider (I am bad at naming things) class is to the tick system that filters the returned strings from the Formatter instances, but that is a pretty major change.

👍 on merging this now and sorting out better ergonomics to control which ticks are labeled later.

@tacaswell tacaswell merged commit 4d692d8 into matplotlib:v2.x Nov 8, 2016
@QuLogic QuLogic changed the title [MRG+1] ENH: restore default ability to label some minor log ticks. ENH: restore default ability to label some minor log ticks. 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.

None yet

3 participants