Skip to content

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

Closed

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Sep 12, 2018

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

  • make just in axis.py and change the Locator later...

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer
Copy link
Contributor

anntzer commented Sep 13, 2018

The image diffs look worse? (a tick that I think we want went away)

@jklymak
Copy link
Member Author

jklymak commented Sep 13, 2018

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?

@anntzer
Copy link
Contributor

anntzer commented Sep 13, 2018

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).
In fact running the specgram example as of master showcased another funny issue: the tick at 0.0 is present when my window is at its default (rcparam default) size, but disappears if I maximize the window...

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Sep 13, 2018

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.

@jklymak
Copy link
Member Author

jklymak commented Sep 13, 2018

The use case I know of where you want more ticks is automatic levels for contour where you want the levels to bracket vmin and vmax. So I kept a kwarg for this case trim_outside=False

@efiring
Copy link
Member

efiring commented Sep 13, 2018

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.

@efiring
Copy link
Member

efiring commented Sep 13, 2018

This looks like the origin of the half-pixel adjustment: 255f213

@jklymak
Copy link
Member Author

jklymak commented Sep 13, 2018

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.

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.

@jklymak
Copy link
Member Author

jklymak commented Sep 13, 2018

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

@jklymak
Copy link
Member Author

jklymak commented Sep 13, 2018

Looking at the code in axis it seems to me the proper thing to do is to make transforms.interval_contains actually be floating-point robust....

I took out the interval_expanded stuff on master out of axis.py (i/e/ w/o this PR), and it all passed test_axes except for the specgram examples. So, I kind of think its obviated (maybe a 64-bit thing?) I think

  1. that code should go (a bunch fewer transforms!)
  2. interval_contains should be made floating point robust.
  3. MaxNLocator should only return ticks between vmin and vmax according to the robust interval_contains

@@ -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
Copy link
Member Author

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.

@jklymak jklymak force-pushed the api-change-MaxNLocator-trim branch 2 times, most recently from 9ca82e1 to 3aee68d Compare September 14, 2018 21:01
@tacaswell tacaswell added this to the v3.1 milestone Sep 15, 2018
@jklymak jklymak force-pushed the api-change-MaxNLocator-trim branch from 550a981 to 73adcac Compare September 17, 2018 16:39
@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2019

I think this PR is obsolete now...

@jklymak jklymak closed this Jan 30, 2019
@jklymak jklymak deleted the api-change-MaxNLocator-trim branch January 30, 2019 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants