Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: make sure we have more than 1 tick with small log ranges#18754
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
If we have both a small target number of ticks and a small displayedrange we would previously pick a tick stride that was the same size orbigger than the visible data range which would result in only 1 tickbeing visible on the axis.This patch ensures that, with a floor at 1, the stride is smaller thanthe estimated data range.re-closesmatplotlib#8768 (same symptoms, different cause)
| @@ -2479,6 +2479,13 @@ def tick_values(self, vmin, vmax): | |||
| if mpl.rcParams['_internal.classic_mode'] else | |||
| (numdec + 1) // numticks + 1) | |||
| # if we have decided that the stride is as big or bigger than | |||
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.
Is this really the right solution to the original problem? To me, it seems thatnumticks is 1 because there isn't room for more ticks. Do we really want ticks that overwrite each other for very narrow axes?
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.
If you are very narrow, there's no good solution. Either you have only one tick, which makes your scale uninterpretable. Or you have overlap. 🤷
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, let me rephrase: We clip numticks to 2, so why don't we have 2 ticks?
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.
numticks is an upper bound and only "best effort" at that.
The issue is that based onnumtick and a rounded "in" version of the range we pick a stride between major ticks (which is meant to catch things like 30 orders of magnitude with 9 ticks). If we propose 3 ticks, have a range of [5, 150], then we "round out" to [1, 100] for the range giving usnumdec = 2 andnumticks = 3 and a stride of(3 / 3) + 1 == 2 which means we put ticks at(10 ^ {-2, 0, 2, 4}) which in turn means that only the tick at 100 is actually shown. Additionally because we only show the minor ticks if the major stride is equal to 1 you end up with exactly 1 tick and 1 label shown even though the users / auto logic asked for up to 3.
I think that this fix (which fixes the values after doing a bunch of heuristics) is a better and more reliable fix than making the heuristics more complicated.
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.
Does this fix the minor tick issue?
Mostly. The issue I described can be pushed up a bit to hit the case where we should have ~2 decades in view and we pick a 3 decade stride and in that case we still don't have the minor ticks, but will at least get 2 major major ticks. I did some manually testing with this by making the y-axis super small and then interactively panning / zooming to see if anything bad looking happened... |
… with small log ranges
…754-on-v3.3.xBackport PR#18754 on branch v3.3.x (FIX: make sure we have more than 1 tick with small log ranges)
| ll.set_params(numticks=numticks) | ||
| for top in [5, 7, 9, 11, 15, 50, 100, 1000]: | ||
| ticks = ll.tick_values(.5, top) | ||
| assert (np.diff(np.log10(ll.tick_values(6, 150))) == 1).all() |
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.
@tacaswell There seems to be a clear typo in the test here: you don't do anything withticks, and the assert is the same on each loop iteration. Do you remember what you intended to do?
PR Summary
If we have both a small target number of ticks and a small displayed
range we would previously pick a tick stride that was the same size or
bigger than the visible data range which would result in only 1 tick
being visible on the axis.
This patch ensures that, with a floor at 1, the stride is smaller than
the estimated data range.
re-closes#8768 (same symptoms, different cause)
attn@aggna
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).