Fix lost ticks #1686

Merged
merged 12 commits into from Feb 19, 2013

6 participants

@dhyams

For issue #1310: Drops last tick label for some ranges

Added a strategy to stop losing ticks on the ends of an axis. The approach that is
used is to pad the interval that is being used to "clip" ticks from the axis because
they are off the end.

The padding is chosen specifically such that the tick cannot be drawn a pixel past the
end of the axis. So long as the tick is within this bound, it should be OK to draw it.

Daniel Hyams added some commits Jan 19, 2013
Daniel Hyams Added a strategy to stop losing ticks on the ends of an axis. The app…
…roach that is

used is to pad the interval that is being used to "clip" ticks from the axis because
they are off the end.

The padding is chosen specifically such that the tick cannot be drawn a pixel past the
end of the axis.  So long as the tick is within this bound, it should be OK to draw it.
255f213
Daniel Hyams Modifications to MultipleLocator and LogLocator, so that the locators…
… will

give locations one past what they really need to.  This is necessary, because
in situations where round off error matters, we want the locator to offer
more that it really thinks it needs to.  The clipping of ticks still takes
place in the Axis class, so it's perfectly fine to add more locs than necessary.

In fact, matplotlib relies on the clipping behavior in the Axis class already
to not draw ticks outside of the limits of the axis.
4c0f3f3
Daniel Hyams oops, missed a return statement in my copy/pasting. 63a40e4
@WeatherGod
Matplotlib Developers member
@dhyams

I just saw that there are five fails in the Travis build. Two of these are by design...the MultipleLocator and LogLocator now return one more location on each end of the axis than they used to.

The other three are image fails, and I don't know how to access the before/after images.

@mdboom
Matplotlib Developers member

Unfortunately, with Travis there isn't a way to access the images -- you just have to run the tests again locally when something fails.

The other failure seems to be that the theta ticking on a polar plot is somehow broken. The one with the missing theta labels is from this branch -- the other is from the baseline images.

polar_rmin
polar_rmin-expected

@dhyams

Thanks Michael: I've installed nose and other dependencies, and I'm running the tests now. Good thing for the nose, or the polar thing would not have been caught ;)

Daniel Hyams Fixed two of the failing tests for this "Fix lost ticks" branch, and …
…fixed a problem

in the calculation of the log tick locations.
1f3959d
@efiring
Matplotlib Developers member
@dmcdougall
Matplotlib Developers member

Leaving a note here for github linkage foo: this PR is an alternative to PR #1659.

Daniel Hyams added some commits Jan 27, 2013
Daniel Hyams Fix the nose failure for polar plots; don't try to pad the interval for
the theta axis of a polar plot.
a0a7998
Daniel Hyams Since the _get_pixel_distance_along_axis routine is supposed to retur…
…n a distance

in data coordinates, it should return 0.0 instead of 0 when appropriate.
82db1d6
@mdboom
Matplotlib Developers member

Just a note -- the Travis failures seem to be identical to those on master -- so this is "passing" by some description, at least.

@dhyams

Right; they seem to be some weird difference in fonts; perhaps the reference images just need to be updated?

@dhyams

On this one, is there anything else that I need to do?

@pelson pelson commented on an outdated diff Feb 11, 2013
lib/matplotlib/ticker.py
@@ -1165,7 +1165,7 @@ def tick_values(self, vmin, vmax):
vmin = self._base.ge(vmin)
base = self._base.get_base()
n = (vmax - vmin + 0.001 * base) // base
- locs = vmin + np.arange(n + 1) * base
+ locs = vmin-base + np.arange(n + 3) * base
@pelson
Matplotlib Developers member
pelson added a note Feb 11, 2013

As per PEP8, there should be spaces either side of the minus operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on an outdated diff Feb 11, 2013
lib/matplotlib/ticker.py
@@ -1450,8 +1450,8 @@ def tick_values(self, vmin, vmax):
while numdec / stride + 1 > self.numticks:
stride += 1
- decades = np.arange(math.floor(vmin),
- math.ceil(vmax) + stride, stride)
+ decades = np.arange(math.floor(vmin)-stride,
@pelson
Matplotlib Developers member
pelson added a note Feb 11, 2013

And here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on an outdated diff Feb 11, 2013
lib/matplotlib/ticker.py
@@ -1450,8 +1450,8 @@ def tick_values(self, vmin, vmax):
while numdec / stride + 1 > self.numticks:
stride += 1
- decades = np.arange(math.floor(vmin),
- math.ceil(vmax) + stride, stride)
+ decades = np.arange(math.floor(vmin)-stride,
+ math.ceil(vmax) + 2*stride, stride)
@pelson
Matplotlib Developers member
pelson added a note Feb 11, 2013

And the multiplication operator here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on an outdated diff Feb 11, 2013
lib/matplotlib/axis.py
@@ -1599,6 +1612,30 @@ def _get_offset_text(self):
self.offset_text_position = 'bottom'
return offsetText
+ def _get_pixel_distance_along_axis(self,where,perturb):
+ # returns the amount, in data coordinates, that a single pixel corresponds to in the
@pelson
Matplotlib Developers member
pelson added a note Feb 11, 2013

Could be a real docstring here instead of a comment...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson and 2 others commented on an outdated diff Feb 11, 2013
lib/matplotlib/axis.py
@@ -1599,6 +1612,30 @@ def _get_offset_text(self):
self.offset_text_position = 'bottom'
return offsetText
+ def _get_pixel_distance_along_axis(self,where,perturb):
+ # returns the amount, in data coordinates, that a single pixel corresponds to in the
+ # locality given by "where", which is also given in data coordinates, and is an x coordinate.
+ # "perturb" is the amount to perturb the pixel. Usually +1 or -1.
+
+ # first figure out the pixel location of the "where" point. We use 1e-10 for the
+ # y point, so that we remain compatible with log axes.
+ #
+ # Note that this routine does not work for a polar axis, because of the 1e-10 below. To
+ # do things correctly, we need to use rmax instead of 1e-10 for a polar axis. But
+ # since we do not have that kind of information at this point, we just don't try to
+ # pad anything for the theta axis of a polar plot.
+ #
+ if self.axes.name == 'polar':
@pelson
Matplotlib Developers member
pelson added a note Feb 11, 2013

I don't really like this approach. What if I wanted to implement my own projection and it didn't even have an inverted data transformation? The safer, though perhaps not best, option would be to make sure self.axes were rectangular (I'd be happy with that).

@dhyams
dhyams added a note Feb 11, 2013

Hmm; I think the approach is correct here...the only problem with the polar projection is the singularity at r=0, which is why it is specifically exempted. The code should activate for everything else.

As for not having an inverted transformation, would that even make sense? Not being able to go from pixel coordinates to data coordinates? Seems like there would be quite a few other things that would break in that situation; not just this.

@mdboom
Matplotlib Developers member
mdboom added a note Feb 11, 2013

I think @pelson's point might be that it's not a great idea to single out a particular projection here -- the user can install and use any number of projections. I haven't given it much thought, but it would be great to find a way to detect this case other than just checking for polar.

@dhyams
dhyams added a note Feb 11, 2013

The singling out, though, is for the one projection that I know it doesn't work for. It should work for others, but just for safety, in the latest commits, I wrapped a try/except around each call that gets the amount of padding to use, which defaults the padding to zero in the case of a problem. This recovers old matplotlib behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Matplotlib Developers member

We're trying to standardize the codebase on PEP8, so would you mind going through and putting things like spaces between operators and after commas etc.

Cheers,

@dhyams

Sure, I'll modify it tonight and push again. I do wish, though, that there was a reasonable workflow such that others could make minor edits to changeset like this...in a fraction of the time required to post comments, it could have just been corrected to conform to pep8. Now I put it on my to-do list; pull up the vm when I get home, reread my git cheat sheet, make a couple of tiny edits, repush, verify that the changes made it, and await comment again. Seems like a very inefficient process, when it would be ideal just to make the edits right here in github. Just wondering if it were even possible. OK, enough of my whining ;) I don't mean it in a derogatory sense...just wondering if a more efficient process is available.

@mdboom
Matplotlib Developers member

You can edit directly on github, on your own branch. Just go to your fork (github.com/dhyams/matplotlib), choose the branch related to this pull request, browse to the file, press the Edit button... it will let you edit the commit message, and the changes will automatically show up in this pull request.

As another time saver (which only works if you have the code checked out on your local machine), I use this which adds a "open in editor" button toevery comment on github, which opens up the given file to the correct line in my editor. The example is with emacs, but there's nothing about the approach that couldn't be used with most other editors.

dhyams added some commits Feb 11, 2013
@dhyams dhyams PEP8 fixes/protection try/except
PEP8 fixes, and part of a comment changed to a docstring.
Also added a try/except around each call to _get_pixel_distance_along_axis, so that
if something goes wrong with some off-beat transform, we just don't pad; things
revert back to old matplotlib behavior.
e2a6f92
@dhyams dhyams PEP8 fixes. c1b9886
@dhyams

Thank you mdboom; I had no idea that you could edit and commit like that. Of course, doing so makes it blindingly obvious that the changes are not tested, right ;)

@efiring efiring commented on an outdated diff Feb 17, 2013
lib/matplotlib/axis.py
@@ -1599,6 +1618,30 @@ def _get_offset_text(self):
self.offset_text_position = 'bottom'
return offsetText
+ def _get_pixel_distance_along_axis(self,where,perturb):
@efiring
Matplotlib Developers member
efiring added a note Feb 17, 2013

Here and below, PEP8 spaces after commas, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring efiring and 1 other commented on an outdated diff Feb 17, 2013
lib/matplotlib/axis.py
@@ -972,11 +972,30 @@ def _update_ticks(self, renderer):
tick_tups = [ti for ti in tick_tups
if (ti[1] >= ilow) and (ti[1] <= ihigh)]
+ # so that we don't lose ticks on the end, expand out the interval ever so slightly. The
+ # "ever so slightly" is defined to be the width of a half of a pixel. We don't want to draw
+ # a tick that even one pixel outside of the defined axis interval.
+ if interval[0] <= interval[1]:
+ interval_expanded = interval
+ else:
+ interval_expanded = interval[1], interval[0]
+
+ if hasattr(self,'_get_pixel_distance_along_axis'):
+ try:
+ ds1 = self._get_pixel_distance_along_axis(interval_expanded[0], -1) / 2.0
@efiring
Matplotlib Developers member
efiring added a note Feb 17, 2013

This could be simplified a bit by using plus or minus 0.5 as "perturb", correct?

Also, you could leave out the "hasattr" check, since the other try/except blocks will catch the same thing.

More generally, however, we try hard to avoid having try/except blocks that catch anything. Maybe there is no good alternative here. If so, a short comment might be warranted.

Is there a need for two blocks? Could everything be condensed into a single block, with the idea that if something goes wrong with either end of the interval, chances are it will go wrong with the other, and there is not much point in trying to expand only one end?

@dhyams
dhyams added a note Feb 18, 2013

Re: simplification...yes, and that's actually correct for a nonlinear axis (like the log axis), while what I did was close, but not correct. I'll fix that.

Re: hasattr check: I personally would rather leave that in, as it signals to a projection author that this method is recommended but optional.

Re: catching everything. I don't like it either in general, but I do think that it's the appropriate thing to do in this circumstance. The approach is to pad the interval if at all possible, but if not, just don't pad. Much better to potentially lose a tick on the end of the interval, than a stack trace (in case anything goes wrong in the padding).

Re: two blocks; I like the two blocks, because a floating point exception that gets raised on one end might not on the other, so we get the benefit of the padding at least on one end, in that case.

@efiring
Matplotlib Developers member
efiring added a note Feb 18, 2013

OK, I suggest commenting the code with a highly condensed version of your explanation above. Since the "catch-all" is an intentional exception to the usual rule, it is worth flagging as such, so that some future reader of the code understands why it is being done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring
Matplotlib Developers member

@weathergod, you commented a month ago that you needed to check for 3D implications. Any thoughts on that, or any other aspect of this? It would be good to either get this moving, or reject it.

Daniel Hyams added some commits Feb 18, 2013
Daniel Hyams More PEP8 fixes and comments 36ce184
Daniel Hyams Pixel perturbation is now 0.5, for a more accurate calculation of exa…
…ctly how

much padding to add.
7e608d4
Daniel Hyams Added a comment about the try/except block that catches malfunctions in
the _get_pixel_distance routines.  Also added a warning emit, so that
the try/catch at least says something if it is triggered.
3cf7ea5
@WeatherGod
Matplotlib Developers member

Doesn't seem to have any negative impacts for mplot3d, as far as I can see. Haven't really had the time to examine the change in-depth, but I think the basic idea is sound.

@efiring efiring merged commit 1aa61e3 into matplotlib:master Feb 19, 2013

1 check failed

Details default The Travis build could not complete due to an error
@efiring
Matplotlib Developers member

Let's see how it works in practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment