Can you add a test for this? I don't think this needs to be an image test, just check that the axes limits are correct after calling ax.margins() and ax.autoscale_view()?

I've fixed the PEP8 issues and added a test to test_axes.py. Since the new behavior is only relevant with nonzero margins, I assumed it would be sufficient to check that the limits are correctly updated after calling ax.margins(), without also making a separate call to ax.autoscale_view(). If that assumption is wrong, then of course I can make the appropriate changes.

I would be curious to know if this would also work with semilog scales (or
does it already? I haven't looked at the problem in detail yet)

On Tue, Jul 7, 2015 at 4:22 AM, David Zaslavsky notifications@github.com
wrote:

I've fixed the PEP8 issues and added a test to test_axes.py. Since the new
behavior is only relevant with nonzero margins, I assumed it would be
sufficient to check that the limits are correctly updated after calling
ax.margins(), without also making a separate call to ax.autoscale_view().
If that assumption is wrong, then of course I can make the appropriate
changes.

—
Reply to this email directly or view it on GitHub
#4592 (comment)
.

I would be curious to know if this would also work with semilog scales (or does it already? I haven't looked at the problem in detail yet)

Yes, it automatically works with semilog plots. ax.autoscale_view() calculates the view limits separately for each axis. Each calculation is independent of the other: the calculation of view limits for the horizontal axis does not need to know or care whether the vertical axis is linear or logarithmic, and vice versa.

When writing the test case I originally included all four plot types - plot(), semilogx(), semilogy(), and loglog() - but I left the semilog plots out of the final test because plot() and loglog() are enough to cover all branches. I couldn't think of any error that would be caught by testing a semilog plot but not by either the linear or log-log test.

Sorry, not semilog, I got myself confused. I meant symlog.

On Tue, Jul 7, 2015 at 3:54 PM, David Zaslavsky notifications@github.com
wrote:

I would be curious to know if this would also work with semilog scales (or
does it already? I haven't looked at the problem in detail yet)

Yes, it automatically works with semilog plots. ax.autoscale_view()
calculates the view limits separately for each axis. For example, when it's
applied to a semilogy() plot, it chooses the linear margin calculation
algorithm for the horizontal axis and the logarithmic algorithm (the one I
added) for the vertical axis.

When writing the test case I originally included all four plot types -
plot(), semilogx(), semilogy(), and loglog() - but I left the semilog
plots out of the final test because plot() and loglog() are enough to
cover all branches. I couldn't think of any error that would be caught by
testing a semilog plot but not by either the linear or log-log test.

—
Reply to this email directly or view it on GitHub
#4592 (comment)
.

I suspect the ideal way to implement this is via transforms: calculate the margin in axes coordinates, and use the transform to convert to data coordinates. Without something like this, there will be increasing numbers of cases where the result will not be as intended. I'm not sure what difficulties would be encountered in this approach, though.

When I authored the commit I didn't know there were any other options beyond linear and log scales... so I'm pretty sure symlog at least will need another branch of the if statement. If there can be arbitrary transforms then it becomes even more complicated, and the implementation probably has to be completely changed.

I'm not familiar with the capabilities of transforms in matplotlib offhand, but I'll look into it tomorrow. It might still be straightforward.

Don't forget polar plots! Yeah, I think Eric got the right idea. If you are
feeling ambitious: http://matplotlib.org/users/transforms_tutorial.html

On Tue, Jul 7, 2015 at 4:06 PM, David Zaslavsky notifications@github.com
wrote:

When I authored the commit I didn't know there were any other options
beyond linear and log scales... so I'm pretty sure symlog at least will
need another branch of the if statement. If there can be arbitrary
transforms then it becomes even more complicated, and the implementation
probably has to be completely changed.

I'm not familiar with the capabilities of transforms in matplotlib
offhand, but I'll look into it tomorrow. It might still be straightforward.

—
Reply to this email directly or view it on GitHub
#4592 (comment)
.

Working on it. I actually rather like messing with transforms, so I'll go ahead and do it that way - it will just take a bit of care.

One other thing: I've noticed that ax.set_xlim() and ax.get_xlim() and their y-axis equivalents are documented as getting/setting the data limits for the plot, but according to the code they actually get/set the view limits. Unless I'm missing something, I think the documentation should be updated, but I'm not sure whether that belongs as part of this pull request or it should be raised as a separate issue.

I think that data limits band view limits are synonyms in this case, but
it has been a while sense i read this section of the code in detail.

On Wed, Jul 8, 2015, 3:09 AM David Zaslavsky notifications@github.com
wrote:

Working on it. I actually rather like messing with transforms, so I'll go
ahead and do it that way - it will just take a bit of care.

One other thing: I've noticed that ax.set_xlim() and ax.get_xlim() and
their y-axis equivalents are documented as getting/setting the data
limits for the plot (in the method docs and also in the transforms
tutorial), but according to the code
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/axes/_base.py#L2553
they actually get/set the view limits. Unless I'm missing something, I
think the documentation should be updated, but I'm not sure whether that
belongs as part of this pull request or it should be raised as a separate
issue.

—
Reply to this email directly or view it on GitHub
#4592 (comment)
.

After a couple days of looking at this, I think margins for arbitrary projections may just be too complicated to do in general, given the current structure of the code.

For a rectangular plot, it's pretty straightforward: compute the view limits without accounting for margins, then transform (-marginx, -marginy) and (1 + marginx, 1 + marginy) from axis coordinates back to data coordinates, then use the result to update the view limits again. But for an arbitrary projection, there's no guarantee that (-marginx, -marginy) and (1 + marginx, 1 + marginy) are the axis coordinates that correspond to the data limits we need to set. They might not even be inside the plot area. With a polar plot, for example, if rmax == 1 and the margin is 0.5, the point (-marginx, -marginy) in axis coordinates corresponds to theta = 5*pi/4 and r = 1.5*sqrt(2), which is completely unrelated to the data limits we'd need to set to implement the margin. (I think everyone would agree a sensible margin implementation would entail increasing rmax to 1.5, and no other changes.)

I think in an ideal world, the most sensible way to implement margins for nearly-arbitrary projections would be setting the initial zoom factor to larger than 1. Projections already have some concept of zooming because it's part of the API, and it seems likely that a projection which doesn't support zooming at all also can't meaningfully be given a margin. However there's currently no general set_zoom() method that could be invoked to do this. For a polar plot, for example, the zooming is done manually in drag_pan() by calculating and resetting rmax. If there were a set_zoom() method, presumably it could be relied on to do the right thing for each projection: in a polar projection, it sets rmax; in a linear projection, it sets xmax, xmin, ymax, and ymin; for other projections, it would do whatever the proper equivalent is.

Alternatively, we could implement a margin by inserting a scale factor and translation (equivalent to a rescaling centered on (0.5, 0.5) in axis coordinates) into the transformation pipeline prior to ax.transAxes. I haven't yet gotten to understand the transformation system well enough to know what implications that would have, though. E.g. if it would also involve changing the axis transformations for rectangular plots, or if it would cause the plot boundaries to be drawn incorrectly. Besides, if I understand how it works correctly, transAxes is not a TransformWrapper so there's no way to insert that scaling into the pipeline in a generic way that works for all transforms.

Anyway, those are just some thoughts. I'd be interested to hear responses. I think the issue of margins for a general projection might have to wait, and for now we settle for handling separable projections, as well as polar axes as a special case.

For now I have to figure out why the new implementation of autoscale_view breaks a bunch of contour plot tests, then I'll get it added to the pull request.

@diazona Thanks for thinking carefully about this.

As something of a side comment, it is not even clear to me that we are handling zoom and pan in polar axes in a useful way; in fact, I don't find it useful at all. What I would want to do with a polar plot is not zoom in on the origin, but zoom in on a feature that could be anywhere; the zooming and panning should be in display space, not r-theta space. The aspect ratio should be preserved, but otherwise it would not differ from zoom in a Cartesian plot. I think this is consistent with your idea of how margins might be implemented as part of a general zoom strategy.

The latest set of commits implement @efiring's proposal for computing margins using transforms. It works for linear, log, and symlog scales, and supposedly should handle any other separable projection. For nonseparable projections it raises NotImplementedError. I'd be open to adding polar plot handling by scaling rmax, but it would be a special case for polar plots only.

Oh, and I definitely agree that the current handling of zoom in polar plots doesn't make a whole lot of sense. But the changes required to fix zooming and margins for nonseparable projections would be extensive, and probably a matter for a separate pull request.

Just checking back in on this PR, since it's been "needs_review" for a while: is there anything else that I need to do for it?

attn @efiring again

It looks to me like the main idea here has been implemented independently in 41d7e12, #5583: transforms and inverse transforms are being used to apply the margins in axes coordinates. @diazona, I'm sorry your PR languished so long.

The comment that set_xlim should be described as setting the view limits, not the data limits, remains valid.

Are there projections for which autoscale_view is not implemented? It is defined in axes._base, so I would not think so.

Yes, it's a shame it sat around so long, but if the main idea been implemented anyway, I guess this PR has served its purpose. That works for me :-) The thing about the documentation can be handled in a separate bug report.