Skip to content

API: make MaxNLocator trim out-of-view ticks before returning#12105

Closed
jklymak wants to merge 9 commits into
matplotlib:masterfrom
jklymak:api-change-MaxNLocator-trim
Closed

API: make MaxNLocator trim out-of-view ticks before returning#12105
jklymak wants to merge 9 commits into
matplotlib:masterfrom
jklymak:api-change-MaxNLocator-trim

Conversation

@jklymak

@jklymak jklymak commented Sep 12, 2018

Copy link
Copy Markdown
Member

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

anntzer commented Sep 13, 2018

Copy link
Copy Markdown
Contributor

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

@jklymak

jklymak commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

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

anntzer commented Sep 13, 2018

Copy link
Copy Markdown
Contributor

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

ImportanceOfBeingErnest commented Sep 13, 2018

Copy link
Copy Markdown
Member

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

jklymak commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

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

efiring commented Sep 13, 2018

Copy link
Copy Markdown
Member

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

efiring commented Sep 13, 2018

Copy link
Copy Markdown
Member

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

@jklymak

jklymak commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

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

jklymak commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

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

jklymak commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

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

Comment thread lib/matplotlib/axis.py
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
Copy Markdown
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
@tacaswell tacaswell added the API: changes Changes to the public API, typically requiring deprecation. label Sep 15, 2018
@jklymak jklymak force-pushed the api-change-MaxNLocator-trim branch from 550a981 to 73adcac Compare September 17, 2018 16:39
@jklymak

jklymak commented Jan 30, 2019

Copy link
Copy Markdown
Member Author

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

Labels

API: changes Changes to the public API, typically requiring deprecation. status: work in progress topic: ticks axis labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants