Caching the results of Transform.transform_path for non-affine transforms #723

pelson · 2012-02-27T17:49:20Z

This pull request supports the previously intended capability of caching non-affine transformations. It was discussed briefly on the devel mailing-list:

http://old.nabble.com/Caching-the-results-of-Transform.transform_path-for-non-affine-transforms-td33256890.html

An example of how to test this capability by hand was included on the thread:

import matplotlib.transforms


class SlowNonAffineTransform(matplotlib.transforms.Transform):
    input_dims = 2
    output_dims = 2
    is_separable = False
    has_inverse = True

    def transform(self, points):
        return matplotlib.transforms.IdentityTransform().transform(points)

    def transform_path(self, path):
        # pretends that it is doing something clever & time 
        # consuming, but really is just sleeping
        import time
        # take a long time to do something
        time.sleep(3)
        # return the original path
        return matplotlib.transforms.IdentityTransform().transform_path(path)

if __name__ == '__main__':
    import matplotlib.pyplot as plt

    ax = plt.axes()
    ax.plot([0, 10, 20], [1, 3, 2], transform=SlowNonAffineTransform() + ax.transData)
    plt.show()

pelson · 2012-02-27T17:59:44Z

@WeatherGod said on the mailinglist:

Chances are, you have just now become the resident expert on Transforms.

A few very important questions.  Does this change any existing API?  
If so, then changes will have to be taken very carefully.  
Does all current tests pass?  
Can you think of any additional tests to add (both for your changes and for the current behavior)? 
How does this impact the performance of existing code?

Maybe some demo code to help us evaluate your use-case?

Answering inline:

Does this change any existing API?
No, as far as I can tell this is entirely backwards compatible. (other than the fact that TransformWrapper now exposes its correct affine status).
Do all current tests pass?
No, but they weren't passing on my machine before hand. Would you mind checking on your machine?
Can you think of any additional tests to add (both for your changes and for the current behavior)?
Yes, I will do that shortly.
How does this impact the performance of existing code?
This is my biggest concern, primarily because it is run so often. Is there any way you know of to compare the performance?

jdh2358 · 2012-02-27T18:03:30Z

Hi Phil,

I wonder if something is wrong with your pull request. I am seeing 17 commits in the history, many of which do not seem relevant to this PR. Eg, many of them are from your as_mpl_transform work, etc. Also, if you will be re-working the PR, is there a logical place to put some of your discussion from the mailing list in the comments or docs. I'd like to see more of this institutional knowledge make it into the code base.

BTW, here is my cheatsheet for how I handle git branches, remotes, etc

https://github.com/jdh2358/notes/blob/master/git_workflow.rst

pelson · 2012-02-28T09:55:06Z

Hopefully this has tidied up the branch. The result of git log looks like it should & the pull request is showing the correct details. If there are any problems with it I will make a new branch, cherry pick and open a new pull.

WeatherGod · 2012-02-28T15:53:11Z

look like it has tidied it up. The addition of tests to make sure this behavior works as expected and doesn't break in the future will be very nice. As for a performance check, my guess is as a zero-th order check would be the timing of the doc build (from a completely clean slate) and/or the timing of running the tests. A slightly better estimate might involve some of the user-interaction "test" code I have made for mplot3d and combine that with profiling to measure differences when doing panning and zooming.

Heck, one could devise a setup where one would use cprofile on some test code that exercises the transform stack and do a betore/after analysis of the profile results.

pelson · 2012-03-02T13:55:43Z

lib/matplotlib/transforms.py

@@ -1286,6 +1287,7 @@ def _set(self, child):
        self.transform_path_non_affine = child.transform_path_non_affine
        self.get_affine                = child.get_affine
        self.inverted                  = child.inverted
+        self.is_affine                 = child.is_affine


This line is causing issues which I don't understand:

import matplotlib.pyplot as plt plt.scatter(range(10), range(10)) plt.show()

Commenting it out fixes the problem, but I don't feel comfortable with that approach, I would like to understand the problem. Any ideas?

b28b972 Fixed this issue. Code testing isinstance(trans, IdentityTransform) is not safe since TransformWrapper could be wrapping an Identity transform, therefore I changed the code to use transform equality instead.

pelson · 2012-03-06T17:02:50Z

I wanted to share some performance findings:

import matplotlib.pyplot
import timeit
import numpy

cmd = 'ax.transData.invalidate()'
setup = 'import matplotlib.pyplot as plt; ax = plt.axes(); ax.plot(range(10)); plt.draw()'
timer = timeit.Timer(cmd, setup=setup)

print timer.repeat(3, 100000)

On master:

[0.1190330982208252, 0.11953496932983398, 0.12048912048339844]

On this branch

[3.766791820526123, 3.8515989780426025, 3.927263021469116]

Clearly this branch reduces the performance of invalidation significantly (~x40 slower), but even so, 100,000 invalidations are completed in ~3.8 seconds - and it is worth remembering that the reason master is so fast is that it is implementing an incorrect invalidation mechanism.

I have been using this branch significantly in the last couple of weeks and found the tactile performance to be identical to master, even so, I am sure that someone else would like to experience its interactive performance for themselves.

I am in the process of writing unit tests for this change which I will push accordingly, but do not intend to look at performance any further unless there is further demand to do so.

Many Thanks,

mdboom · 2012-03-19T14:34:52Z

Sorry to take so long to get to this. It seems like this changes are appropriate and correct. The only change I would request would be to not create a new test module (test_plot_types.py), but to put the new test in test_axes.py. I'm now going to turn my attention to #731.

mdboom · 2012-03-19T15:32:56Z

Also, as a side note: This is a bona fide bug that needs fixing, but the solution is rather invasive, so I would recommend (just as it's already set up) merging this to master and not the 1.1.x bugfix branch.

pelson · 2012-04-20T17:06:18Z

@mdboom: My latest commit re-addresses a bug I couldn't get to the bottom of at b28b97208c054206232d4d18e9c9903c847c12c2 but as you will see, it required a fudge (in 3f9479e650f58701b509969520e2620f9fce2550). I think there is something wrong with agg_py_transforms.cpp but cpp is not my speciality. Would you mind providing some assistance?

Note: This pull request should not be merged until the latest commit is resolved.

pelson · 2012-04-20T17:07:39Z

@mdboom: I should have said, you need to comment out the lines around # XXX INVESTIGATE. and simply run:

import matplotlib.pyplot as plt

plt.scatter(range(10), range(10))

plt.show()

pelson · 2012-05-01T12:24:17Z

I have managed to get to the bottom of the problem (I hadn't defined __array__ properly).

The changes to src/agg_py_transforms.cpp should be non-functional changes, just improvements in error messages. As I mentioned, c++ is not one of my specialities so if you spot anything strange, I assure you it wasn't intentional and I would be happy to change it.

pelson · 2012-05-01T12:28:56Z

This pull request is now back at a state where I am happy to propose it for integration into master (obviously after suitable review).

Please note that I will be away from a computer for a couple of weeks so won't be able to respond to any review actions/questions immediately.

mdboom · 2012-06-01T15:01:46Z

Sorry this has been "dropped". I think this is an important fix, but a very complicated, low-level and pervasive one. I think we should probably just bite and bullet and merge, with the expectation that if there are unintended consequences we may revert it.

But first -- It seems that the scatter test does not belong in this PR. I don't see how it's related. This should be rebased on the current master and the test suite should be run to make sure this still works against recent changes. If all that passes, I think we're good to merge this.

pelson · 2012-06-07T08:30:59Z

But first -- It seems that the scatter test does not belong in this PR. I don't see how it's related.

Because I tend to be using these changes on a daily basis, I was lucky enough to run a simple scatter plot on the branch (which had all tests passing), and got a exception. This was helpfull in two ways:

It showed that I had a bug (__array__ wasn't defined on one of the Transform classes).
It showed that there was no unit test for a scatter plot.

I'll rebase shortly.

…e problem.

pelson · 2012-06-07T14:31:25Z

I've rebased and tested everything (I'm getting 3 _image_module::readpng: error reading PNG header errors on matplotlib.tests.test_axes.test_polar_coord_annotations.test which I haven't yet got to the bottom of.```

pelson · 2012-06-07T14:33:48Z

lib/matplotlib/backends/backend_svg.py

-            x1, y1 = points[i]
-            x2, y2 = points[(i + 1) % 3]
-            x3, y3 = points[(i + 2) % 3]
+            x1, y1 = tpoints[i]


This bug came out in the pcolor test (the pcolor was not in the correct place). I don't understand how it wasn't failing before, but the fact that the bug has been highlighted by this PR is a good sign.

mdboom · 2012-06-07T14:46:51Z

As for PNG errors -- I wonder if there's a way you could determine what PNG file it was failing on and share that. You may also try a clean rebuild -- distutils does a terrible job at recompiling enough C++ code sometimes.

pelson · 2012-06-07T15:17:24Z

Yes, a clean rebuild did the trick. All tests now pass.

mdboom · 2012-06-07T15:24:33Z

Ok. I say go ahead and merge, if there's no other objections. Thanks for your hard work on this.

pelson · 2012-06-07T15:28:11Z

I'll give it 24 hours before merging.

Caching the results of Transform.transform_path for non-affine transforms

pelson mentioned this pull request Mar 1, 2012

Plot limit with transform #731

Merged

pelson reviewed Mar 2, 2012
View reviewed changes

Phil Elson and others added 9 commits June 7, 2012 15:08

Single commit of necessary transform changes.

753c9a2

Fixed is_affine inheritance & added scatter test which highlighted th…

c4ceace

…e problem.

Fixed failing unit test. (in test_axes)

bf4dd97

Moved scatter test into test_axes.py

4c88a6d

Fixed broken handling of WrappedTransform

c088477

Intermediate commit to request some assistance.

1b1b340

Fixed __array__ releated bug that I introduced.

e9cc98e

Improved comments.

c898bdf

Now working with rebased master.

a81545c

pelson reviewed Jun 7, 2012
View reviewed changes

pelson pushed a commit that referenced this pull request Jun 8, 2012

Merge pull request #723 from pelson/path_transform_cache

67df281

Caching the results of Transform.transform_path for non-affine transforms

pelson merged commit 67df281 into matplotlib:master Jun 8, 2012

mdboom mentioned this pull request Jun 11, 2012

Fix tests following test_non_affine_caching, which was leaving a figure behind #943

Merged

pelson mentioned this pull request Sep 14, 2012

transAxes isn't working properly with scale='log' #1249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caching the results of Transform.transform_path for non-affine transforms #723

Caching the results of Transform.transform_path for non-affine transforms #723

pelson commented Feb 27, 2012

pelson commented Feb 27, 2012

jdh2358 commented Feb 27, 2012

pelson commented Feb 28, 2012

WeatherGod commented Feb 28, 2012

pelson Mar 2, 2012

pelson Mar 6, 2012

pelson commented Mar 6, 2012

mdboom commented Mar 19, 2012

mdboom commented Mar 19, 2012

pelson commented Apr 20, 2012

pelson commented Apr 20, 2012

pelson commented May 1, 2012

pelson commented May 1, 2012

mdboom commented Jun 1, 2012

pelson commented Jun 7, 2012

pelson commented Jun 7, 2012

pelson Jun 7, 2012

mdboom commented Jun 7, 2012

pelson commented Jun 7, 2012

mdboom commented Jun 7, 2012

pelson commented Jun 7, 2012

Caching the results of Transform.transform_path for non-affine transforms #723

Caching the results of Transform.transform_path for non-affine transforms #723

Conversation

pelson commented Feb 27, 2012

pelson commented Feb 27, 2012

jdh2358 commented Feb 27, 2012

pelson commented Feb 28, 2012

WeatherGod commented Feb 28, 2012

pelson Mar 2, 2012

Choose a reason for hiding this comment

pelson Mar 6, 2012

Choose a reason for hiding this comment

pelson commented Mar 6, 2012

mdboom commented Mar 19, 2012

mdboom commented Mar 19, 2012

pelson commented Apr 20, 2012

pelson commented Apr 20, 2012

pelson commented May 1, 2012

pelson commented May 1, 2012

mdboom commented Jun 1, 2012

pelson commented Jun 7, 2012

pelson commented Jun 7, 2012

pelson Jun 7, 2012

Choose a reason for hiding this comment

mdboom commented Jun 7, 2012

pelson commented Jun 7, 2012

mdboom commented Jun 7, 2012

pelson commented Jun 7, 2012