-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
API: make MaxNLocator trim out-of-view ticks before returning #12105
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
The image diffs look worse? (a tick that I think we want went away) |
Specgram doesn’t include 0 in its extents (its 0.2 or something). Should we include ticks that are out of range? If so how far out of range? I’d really argue this is specgrams decision not the decision of the tick locator, but maybe we had the idea that a tick should be made available slightly outside the actual data range? If so how far outside the data range do we want to allow? Is that where all this distance-along-pixel stuff came from? |
Oh sorry, I didn't realize that. In which case I definitely agree with not to put the tick (I think the distance-along-pixel was there for this reason, but only correcting floating point errors). |
It sounds quite natural to only return those tick locations that are within the (closed) interval of [vmin,vmax]. However for some reason there are currently more tick locations returned. I reckon there must be a reason for this; possibly this is a workaround for some rendering problem? In any case I guess it makes sense to find out the reason and then make sure the trunkated version proposed here would not produce the original problem again. |
The use case I know of where you want more ticks is automatic levels for |
Classic-mode autoscaling requires having the ticks outside the range. The autoscaling determines the range such that lower limit is the largest tick location less than or equal to the data vmin, and similarly for the upper an vmax. The original floating-point slop fiddling, later expanded to work in pixel space, was mainly related to this. For example, when the range is large enough that the ticks fall on integers, one doesn't want tiny and accidental floating point deviations from integers to affect the view limits and ticks. |
This looks like the origin of the half-pixel adjustment: 255f213 |
OK, for classic auto scaling we can easily just use the "trim_outside" flag This PR includes a tolerance based on a fraction of vmax-vmin to account for floating point slop. I set it unscientifically at 1e-10, but we could do something larger to let it have more slop. |
The original issue in #1310 was a floating point mismatch with >. While I admit making the tolerance one pixel fixed the floating point error a) it means we have to do some transforms to figure out that size in data space and b) it means the chosen ticks depend on the pixel size. That probably is not practically a problem, but is not very good theoretically. The approach here still scales by the data range but just chooses a small tolerance that is very unlikely to be visible, but is well within floating point tolerance |
Looking at the code in I took out the
|
@@ -1052,50 +1052,8 @@ def _update_ticks(self, renderer): | |||
ihigh = locs[-1] | |||
tick_tups = [ti for ti in tick_tups if ilow <= ti[1] <= ihigh] | |||
|
|||
# so that we don't lose ticks on the end, expand out the interval ever |
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.
interval_contains
below is modified to accept floating point slop, so this doesn't need to be here any more.
9ca82e1
to
3aee68d
Compare
550a981
to
73adcac
Compare
I think this PR is obsolete now... |
PR Summary
Re #11004 This is what I'd do to
MaxNLocator
to make it return just ticks inside the vlim. I've not gone through and changed code where we do this post facto, but its a todo for this PR.Note that I've kept an option when making the locator to continue returning the "extra" locations if desired.
ping @anntzer @efiring for discussion.
Todo
axis.py
and change the Locator later...PR Checklist