Skip to content
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

Support for skewed transforms #1664

Merged
merged 12 commits into from Mar 15, 2014
Merged

Conversation

dopplershift
Copy link
Contributor

The purpose of these changes is to add support for skewed transforms (for the uninitiated, a skewed transform is essentially rotating the x and y axes by different angles). Such transforms are useful for some plot types--one specific such example is the Skew-T in meteorology, which is used to plot vertical profiles of temperature with a skewed x-axis. The x and y axes cross at a (nominally) 30 degree angle--the skewing helps reduce the space occupied by the plot.

To see an example of this work in action, checkout this PR and run the script in this gist.

Overall, no significant changes to matplotlib were required--just adding the transforms and finding little places where assumptions needed to be replaced by more generic calls/attributes. These changes are what I need to be able to make my skewed plots without overriding significant portions of matplotlib, but don't affect (to my knowledge) any existing matplotlib code.

Using these transforms still requires some significant effort (as seen in the gist), mostly due to the fact that the upper and lower x axes have different data ranges. (I'd love to have these in matplotlib too, but it would require massive changes to matplotlib with little gain to most people, and most likely a speed penalty for the genericity.)

Summarizing the changes:

  1. Add skew transforms
  2. Fix pan/zoom for the transform
  3. Fix axhline() and friends
  4. Fix spines
  5. Make spines draw (update) before the axis (ticks)

@dopplershift
Copy link
Contributor Author

An example for the uninitiated (this uses this gist) @WeatherGod should have a look.
example-skewt

@ghost ghost assigned mdboom Jan 16, 2013
@dopplershift
Copy link
Contributor Author

Assigning @mdboom since he did transforms, but I'm not sure who else would be good.

@pmarshwx
Copy link

As someone who nominally worked with @dopplershift on this, and someone who has been using the dopplershift:skew branch as my main mpl version for almost a week now, I have not run across any problems with orthogonal axes plots. However, if I do, I'll be sure to post here.

@dmcdougall
Copy link
Member

I consider @pelson to be a transform ninja.

pts = self.get_points()
ll, ul, lr = transform.transform(np.array([pts[0],
[pts[0, 0], pts[1, 1]], [pts[1, 0], pts[0, 1]]]))
return Bbox([ll, [lr[0], ul[1]]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method change looks quite specialised now. I'm assuming the change was to take into account the skewed (i.e. non-rectilinear) transform and will only work if the skew is in a specific direction? Would it be possible to take the maximum and minimum of all 4 corners to generalise this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, what happens here is that to get a "good" upper right point, it transforms the upper left and lower right points separately and combines them after the transform. I think that was as generic as I could make it.

The bounding box, being in data space, can have the x or y direction inverted, so a simple max/min won't work. Furthermore, in these skewed coordinates, the upper left has a much lower X coord, so min here breaks pan/zoom for me again.

I'm not really sure what bounding box should result from putting a box through a non-rectilinear transformation. I did the best I could by keeping the transformed lower left and assembling a sensible (or at least moreso than the straight transformed one) upper right.

@pelson
Copy link
Member

pelson commented Jan 16, 2013

The changes all look fine to me. I'm not sure if it is my understanding of skew, or whether there is a bug (possibly where I have commented). Please help me interpret my result:

import itertools
import matplotlib.patches as mpatch
import matplotlib.pyplot as plt
import matplotlib.transforms as mtrans


fix, axes = plt.subplots(5, 5, sharex=True, sharey=True, figsize=(16, 12))
axes = axes.flat

rotations = list(itertools.product([-3, -1, 0, 1, 3], repeat=2))

axes[0].set_xlim([-4, 4])
axes[0].set_ylim([-4, 4])

for ax, (xrots, yrots) in zip(axes, rotations):
    xdeg, ydeg = 45 * xrots, 45 * yrots
    t = mtrans.Affine2D().skew_deg(xdeg, ydeg)

    ax.set_title('Skew of {0} in X and {1} in Y'.format(xdeg, ydeg))
    ax.add_patch(mpatch.Rectangle([-1, -1], 2, 2, transform=t + ax.transData,
                                  alpha=0.5, facecolor='coral'))

plt.subplots_adjust(wspace=0, left=0, right=1, bottom=0)
plt.show()

Skew results

@dopplershift
Copy link
Contributor Author

Interesting plot (thanks for giving this a robust test!). I'll have to sit down and stare at this when I have more time.

@mdboom
Copy link
Member

mdboom commented Jan 16, 2013

Great work!

@pelson: Yes, nice test. This (minus the text) should probably be committed as a image comparison test (once we've confirmed everything is correct).

Also, the linked gist should probably be added as an example.

@dopplershift
Copy link
Contributor Author

I'll add a version of the gist. My question inclination is that I should strip it down and remove some of the meteorology specific parts, but how does everyone else feel? When I finally get around to a metr. package, I plan on it living there.

As far as the test is concerned, I'd like to replace/supplement @pelson 's test with a version that uses the skew for the axes data transform. Really, using a skewed transform for the data transform is something that could have been done before just by setting the transform using a matrix--all I've added for that are the methods that generates the matrix. All the tricky corner cases when using it for the Axes itself are what I'd be worried about breaking in the future and IMO really need testing. (I'd disable the top ticks/spine since that's what's even harder and is covered by the gist.)

@WeatherGod
Copy link
Member

I would like to see the skew-T in the gallery, although it can certainly be
a stripped down version. Any meteorologist looking through the gallery
would be able to spot it easily and want to be able to produce it.

@mdboom
Copy link
Member

mdboom commented Jan 17, 2013

@dopplershift: That makes sense, re: the test and example. Thanks.

@dopplershift
Copy link
Contributor Author

Based on some thinking about @pelson 's test and some more research, I've changed the use of the angles in the transform to be shearing angles along each axis. This is more consistent with most of what I've read and easier to understand, IMO. (I also think there might have been unexpected behavior based on the definition as rotation.) This has resulted in the angles being swapped, so I updated the skewT example, as well as @pelson 's example, shown below:
skew_example

Based on the intepretation of the angles as shears, these all look correct, though I'm not sure how much sense shears > 90 make when it comes to the polygon rendering. For the test, I think I'm going to use increments of 90 to avoid these issues and make it more sensitive to proper directions being handled.

@dopplershift
Copy link
Contributor Author

Ok, the more I look at this, I'm having trouble turning @pelson 's example into a good test of the skewT functionality with gridlines and such. The use of subplots is best done with a registered projection, but then I can't set the rotation when the skewed axes are created. Setting the transform objects on the axes after it's created will affect added artists, but that's too late for things like ticks and gridlines.

I'm tempted to take @pelson 's test as is as one test and then add an automated skewT-style test to get a single test of things like gridline, axvline, etc.

Thoughts?

@pelson
Copy link
Member

pelson commented Jan 22, 2013

The use of subplots is best done with a registered projection, but then I can't set the rotation when the skewed axes are created

I added some functionality a couple of months ago (pre v1.2.0) which allows you to define an object with a _as_mpl_axes method.

Docs: http://matplotlib.org/devel/add_new_projection.html#creating-a-new-projection
Simple test case example: https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_axes.py#L798

If you do want to go down this route (I'm not sure its entirely necessary), then you could simply subclass Axes and override _set_lim_and_transforms(self), and then define an object which implements the _as_mpl_axes interface...

@pelson
Copy link
Member

pelson commented Apr 18, 2013

Ok, I think we just need a test on this PR and we are good to go. FWIW The example is really good!

@@ -1834,6 +1837,39 @@ def scale(self, sx, sy=None):
self.invalidate()
return self

def skew(self, xShear, yShear):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8: this should be x_shear and y_shear.

@mdboom
Copy link
Member

mdboom commented Apr 18, 2013

I agreed, looks good. It should probably also have an entry in whats_new.rst.

@pelson
Copy link
Member

pelson commented May 13, 2013

@dopplershift - are you likely to be able to give this the last nudge it needs to get into v1.3.x, or shall we hold fire until v1.4?

@pmarshwx
Copy link

If @dopplershift can't lead this effort, I'll finish it for him. This is something I started with him and would like to see included sooner rather than later.

@pmarshwx
Copy link

I spoke with @dopplershift yesterday and he agreed that I should probably take this over for now as he's pretty slammed at work and we both would like to see this included.

I started to dig into the code last night. The PEP8 fixes were trivial so I've made those. However, I've noticed what I would consider a show stopper at the moment. When you save an image (or display it inline with an IPython Notebook), the images do not appear to be flushed-left, as is the case with "normal" images.

skew

However, if you display this is a GUI window, or save the image from the GUI, everything looks correct.

My guess is that there is something going on with the clipping. The offset to the right appears to be same amount as would be applied if the upper-left most skewed x-axis line (the -100) were extended down to the x-axis. Admittedly, I'm not knowledgeable on the internals of MPL, and in this case, where the clipping is handled. Nor do I understand the differences in clipping between saving a figure and displaying it in a GUI. If anyone can provide me insight into this, I'll attack it.

@mdboom
Copy link
Member

mdboom commented May 15, 2013

@pmarshwx: I'm having trouble reproducing the alignment issue you raise. I checked out this branch, replaced show() with savefig('test.png') at the bottom of skewt.py and got the following image (which doesn't seem to have a large left margin). Is there anything else I would need to do to reproduce the problem?
test

@pmarshwx
Copy link

@mdboom, aha!

Out of habit, I almost always save figures with bbox_inches='tight' set. I don't even think about it. (This is also what IPython Notebook does for inline display.) The issue I listed above appears to be in the calculation of the bbox_inches='tight' setting...which makes sense on some levels.

I don't know much about the bbox_inches caculations, but I'll try to make some time and look into it now that I know from where the problems appear to be coming.

@mdboom
Copy link
Member

mdboom commented May 24, 2013

@pmarshwx: I would love to get this in for 1.3.x -- I hope to tag the release candidate on Tuesday.

@pmarshwx
Copy link

@mdboom: I'll do my best. I'm a meteorologist at the Storm Prediction Center in Norman, OK. With the tornado that hit Moore, OK on Monday, my entire schedule has been thrown into chaos. I'm currently on mids, but hopefully I can work on this Sunday and Monday.

@mdboom
Copy link
Member

mdboom commented May 28, 2013

@pmarshwx : I didn't realize that's where you were. Stay safe! I'm going to tag this for 1.4.x, but once your life returns to normalcy, we can discuss whether it might make more sense for 1.3.1.

@tacaswell
Copy link
Member

@pmarshwx Could you re-base this?

@pmarshwx
Copy link

pmarshwx commented Jan 5, 2014

I'm currently away from a computer (responding from my phone). I had forgotten about this. I'll work on it this evening. If a simple rebase doesn't break things, I'll post tonight. If a rebase breaks things I'll at least let you know.

These set up skew transforms, which can be viewed as indpendantly
rotating the X and Y axes.
This involves transforming the upper left and lower right points
separately to come up with the combined translated upper right.
By default get_[x|y]axis_transform() do the same, but by calling these
methods instead of creating the transforms by hand, we better support
custom Axes.
The manual use of the blended transform breaks non-seperable custom
axes/projections. Removing this does not appear to affect any of the
spine examples.
This gives the spine a chance to update the axis object before draw.
This is sensible given that the axis can be registered with the spine
so that it can modify it, and is necessary for some use cases.
Instead of specifying the angles as rotations of the axes, it is easier
(and more standard) to think of the angles in the skew transform as
shearing angles along each axis. Doing so means the X and Y angles
are swapped from how they were used before, so this commit also
updates the example.
@dopplershift
Copy link
Contributor Author

Ok, code seems to be working here rebased upon master (after the axes refactor). I want to see if I can put together a test real quick....

@dopplershift
Copy link
Contributor Author

Test added to make sure grid, ticks, and axvline works. Anyone have anything else I've missed?

@WeatherGod
Copy link
Member

Need entries for whats_new.rst and api_changes.rst

@dopplershift
Copy link
Contributor Author

Done. Also added @pelson's test with the grid of axes. One question: did anyone see anything else from my example code that could be moved into matplotlib proper? There's a lot of set-up there; most of the PR to this point has been to put in/fix what had to be in matplotlib, but I'd love it if there were more pieces that made sense to move in.

@WeatherGod
Copy link
Member

There is a lot of good setup code there, but I think in order to get this
into 1.4.0 asap, let's just focus on the core parts. We can always iterate
improvements for 1.5.

@tacaswell
Copy link
Member

@WeatherGod Can you be responsible for merging this?

@dopplershift
Copy link
Contributor Author

@WeatherGod I agree that now's not the time to re-work any more of matplotlib. I just wanted to make sure I hadn't missed any low-hanging fruit.

@tacaswell I can merge. I just wanted to let it marinade a couple days to make sure there wasn't anything I missed.

@tacaswell
Copy link
Member

pinged @WeatherGod on the principle one should not merge one's own PR.

@dopplershift
Copy link
Contributor Author

Since there are no objections to this, I'm going to go ahead and merge.

dopplershift added a commit that referenced this pull request Mar 15, 2014
Support for skewed transforms.
@dopplershift dopplershift merged commit d1b13d9 into matplotlib:master Mar 15, 2014
@dopplershift dopplershift deleted the skew branch March 15, 2014 23:04
@@ -517,14 +517,17 @@ def transformed(self, transform):
Return a new :class:`Bbox` object, statically transformed by
the given transform.
"""
return Bbox(transform.transform(self.get_points()))
pts = self.get_points()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dopplershift Just curious, should this fix have been applied to Transform.transform_bbox as well? Right now we have two methods for transforming bboxes with subtly different semantics, it would seem better too only keep one of them...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite possibly? My memory of doing this was that this was what it took to unbreak zooming (and maybe panning) on these skewed plots. It's possible that transform_bbox just isn't used in any code path I've hit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants