@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 runso often. Is there any way you know of to compare the performance?

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

Hopefully this has tidied up the branch. The result ofgit 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.

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.

I wanted to share some performance findings:

import matplotlib.pyplotimport timeitimport numpycmd = '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,

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 intest_axes.py. I'm now going to turn my attention to#731.

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.

@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 withagg_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.

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

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

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

The changes tosrc/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.

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.

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.

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:

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

I'll rebase shortly.

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

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.

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

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

I'll give it 24 hours before merging.