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: always use at least 2 ticks and recompute#5772
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
Tests in the test suite have a different figure size. You can set the size manually when creating the figure, however. |
duh, rcparams for the tests use 'classic' so this functionality is turned off! |
Test failure: |
I can reproduce the inset locator failure locally, looking into it now. |
The issue with the inset locator seems to be that as previously implemented the axis's estimate of how many ticks it should have was computed once and then saved. The way that the inset axes works (if you step through this line at a time) is that the figure-space size of the axes follows the x/y limits at a fixed ratio to the screen-space of the same range in the main axes. Thus, when the inset axes is created it is 'big' in that it has limits of [0, 1] at 6x zoom -> about half the size of the host axes and so it concludes that it needs a whole bunch of ticks and remembers that. One of the changes in this PR makes it so that the number of ticks is recomputed every time, thus many fewer ticks are used when the axes is small (even though the text is all turned off). The simplest solution is to change the test code to not use the auto tick count in the test. Also, kicking my self for merging the PR where this test got added for not asking Mike to change to 'viridis' for the color map. |
I think this is ready for review. |
| @@ -2004,7 +2004,7 @@ def get_tick_space(self): | |||
| # is no more than 3:1 | |||
| size = tick.label1.get_size() * 3 | |||
| size *= np.cos(np.deg2rad(tick.label1.get_rotation())) | |||
| self._tick_space = np.floor(length / size) | |||
| return np.floor(length / size) | |||
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.
So we're no longer caching the calculation? This could be somewhat expensive to do every time. If not too expensive, we should remove theif self._tick_space (and the self._tick_space member) altogether. If it is, maybe we just need to properly invalidate this value using thestale infrastructure (which will be traitlets sometime in the near future)?
@jenshnielsen Finally made the last change to this, it should be ready to go |
Looks good to me. I think the changes to test_axes_grid1 should be implemented in the example that this is based on too i.e. |
For extreme aspect-ratio plots the auto ntick logic would decide thatno ticks will fit, leading to divide by 0 issue. - In ticker ensure there is always at least on bin - Axis do not cache the number of ticks so that if the on-screen aspect ratio changes the number of ticks will update correctly.
Do not let the ticks in the inset adjust based on spacing of text whichis not visible.The issue with the inset locator seems to be that as previouslyimplemented the axis's estimate of how many ticks it should have wascomputed once and then saved.The way that the inset axes works (if you step through this line at atime) is that the figure-space size of the axes follows the x/y limitsat a fixed ratio to the screen-space of the same range in the mainaxes. Thus, when the inset axes is created it is 'big' in that it haslimits of [0, 1] at 6x zoom -> about half the size of the host axes andso it concludes that it needs a whole bunch of ticks and remembers that.One of the changes in this PR makes it so that the number of ticks isrecomputed every time, thus many fewer ticks are used when the axes issmall (even though the text is all turned off).
In the inset axes (which will not have tick labels) fix the numberof ticks instead of letting auto-ticker pick the number of ticks.
@jenshnielsen examples updated. |
cool I will review and merge asap |
FIX: always use at least 2 ticks and recompute
For extreme aspect-ratio plots the auto ntick logic would decide that
no ticks will fit, leading to divide by 0 issue.
aspect ratio changes the number of ticks will update correctly.
I tried to add a test for this, but something about the testing framework is causing trouble
runs correctly as a script for me and interactively, but as part of the test suite give 6 ticks in the first case.