Skip to content

Speeding up contours with (non-affine) transforms #11464

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

Closed
astrofrog opened this issue Jun 20, 2018 · 16 comments
Closed

Speeding up contours with (non-affine) transforms #11464

astrofrog opened this issue Jun 20, 2018 · 16 comments

Comments

@astrofrog
Copy link
Contributor

For the Astropy project, we provide astronomy-specific transforms that users can use in various Matplotlib methods/artists, including e.g. Axes.contour. In fact, we have a sub-class of Axes called WCSAxes which until now just inherited the default contour and contourf. However, the complexity of the transforms means that there is a small overhead (>1-10ms) for every call to the transform that is unrelated to the number of elements to transform.

In Matplotlib, when drawing contours with a transform:

ax.contour(..., transform=...)

each individual path in the contour is passed to transform_path_non_affine. This is done here:

https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/collections.py#L191

However, this means that in cases where a contour map has say 1000 individual lines (not that uncommon with some astronomy images), drawing a contour map can take >10s.

I found a solution in astropy/astropy#7568 - override contour and call matplotlib's contour without the transform, then modify the vertices in-place afterwards by constructing a single array with all the vertices and transforming it in one go and splitting it up again. The key function is:

https://github.com/astropy/astropy/pull/7568/files#diff-421f6873fdd2d8a90f39f3050534b0c3R139

This dramatically sped up drawing contours in our case. The reason I'm opening this issue (as suggested by @dopplershift on Gitter) is that this solution is not specific to Astropy. In fact, this could be copied almost as-is into Matplotlib if there was interest. In our case, there is basically no downside to doing this performance-wise even when there are very few contour lines, and I think this is generally true for this approach.

Feel free to close this if you'd rather not port this over to Matplotlib though, as there may be subtleties I am not aware of!

@jklymak
Copy link
Member

jklymak commented Jun 20, 2018

Is this reproducible with just core matplotlib? i.e. using polar or something? How crazy does the transform have to be, and can we replicate that somehow?

Do we have a way to precompute the transform matrix so that can be passed instead of something that has a bunch of overhead?

I expect there is a way to do the transform on all the contours at once as you've done in your PR, though I'm not at all sure where in the code that goes. But it strikes me as suboptimal to re-pack all the line segments into a single array, do the transform, and then unpack them, rather than just come up with a way to make the transform more efficient.

@jklymak
Copy link
Member

jklymak commented Jun 20, 2018

At the top of contour.py it says that we don't use a single LineCollection for all the contours, but it seems we could re-organize and have a single LineCollection per level. Then the transform overhead would just be proportional to the number of levels instead of the number of line segtments...

# We can't use a single line collection for contour because a line
# collection can have only a single line style, and we want to be able to have
# dashed negative contours, for example, and solid positive contours.
# We could use a single polygon collection for filled contours, but it
# seems better to keep line and filled contours similar, with one collection
# per level.

@astrofrog
Copy link
Contributor Author

astrofrog commented Jun 20, 2018

I agree that things would be much better if Matplotlib just had a single LineCollection per level, and would likely solve the issue for us and remove the need for such a workaround.

@astrofrog
Copy link
Contributor Author

I expect there is a way to do the transform on all the contours at once as you've done in your PR, though I'm not at all sure where in the code that goes. But it strikes me as suboptimal to re-pack all the line segments into a single array, do the transform, and then unpack them, rather than just come up with a way to make the transform more efficient.

In our case it's simply not possible to remove the overhead from the transforms without significantly changing some parts of Astropy. I don't think you'll necessarily see similar overheads with core Matplotlib transforms though. But this is a problem other projects with custom transforms could run into.

@jklymak
Copy link
Member

jklymak commented Jun 20, 2018

I think that the transforms get applied in the c-code for Agg backends. Above my paygrade to know if this could be sped up at this level. A little confused that the c-code would be slow for this, but what do I know...

void _draw_path_collection_generic(GCAgg &gc,

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Jun 20, 2018

the complexity of the transforms means that there is a small overhead (>1-10ms) for every call to the transform that is unrelated to the number of elements to transform.

This sounds a lot like there is room for optimization on the transform part of the problem. Calling a numpy ufunc cannot take 10 ms, right? Creating a ufunc may; but that is something that needs to be done once and not for every transform call.

In general I think contours are not made to produce 1000 levels - that rather seems close enough to just use an interpolated image instead.

@dopplershift
Copy link
Contributor

As a general-purpose plotting library, it's important that, when possible, matplotlib not to impose arbitrary limits on things. Why can't an ordinary image have 1000 contours? Especially in an era of 4k displays, you can have really large images displayed easily. Just because it's not your use case doesn't mean it's not a valid one.

More importantly, we have an existence proof of a more performant approach, which IMO means the current approach in matplotlib is deficient.

@astrofrog
Copy link
Contributor Author

The non-affine transforms get called here in Python, not in C:

https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/collections.py#L191

(this doesn't depend on backend)

@astrofrog
Copy link
Contributor Author

Just to be clear, I'm not talking about 1000s of contour levels but contour lines, i.e. there are multiple separate paths in each contour level. For example the following image:

contour

has a dozen levels, but thousands of individual lines making up the contours.

@astrofrog
Copy link
Contributor Author

Here's an example with an artificial 1ms overhead in the transformation method to try things out:

import time
import numpy as np
import matplotlib.pyplot as plt
from matplotlib.transforms import Transform
from scipy.ndimage import gaussian_filter


class CustomTransform(Transform):

    input_dims = 2
    output_dims = 2
    affine = False

    def transform_path_non_affine(self, path):
        time.sleep(1 / 1000.)
        return path


image = np.random.random((1024, 1024))
image = gaussian_filter(image, 2)

ax = plt.subplot(1, 1, 1)
ax.contour(image, level=10, transform=CustomTransform())
plt.savefig('contours.png')

This has 10 contour levels, and takes 20 seconds to run.

@jklymak
Copy link
Member

jklymak commented Jun 20, 2018

Ha! You should remove the time.sleep from your transform - it'll go way faster! 😆

Seriously, thats very helpful. I think a fix is pretty easy to add to the core along the lines you implimented...

@jklymak
Copy link
Member

jklymak commented Jun 20, 2018

See #11465 for a native fix for this... Note its still slower than what you did, because it only works level by level, but...

@astrofrog
Copy link
Contributor Author

@jklymak - shall we close this since it seems this approach won't work in Matplotlib?

@github-actions
Copy link

github-actions bot commented May 8, 2023

This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 8, 2023
@greglucas
Copy link
Contributor

The single LineCollection idea is implemented here: #25247 which fixes the slow example given in this comment: #11464 (comment)

@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label May 9, 2023
@anntzer
Copy link
Contributor

anntzer commented Jul 12, 2023

Assuming #11465 isn't really viable (per #11465 (comment), which I agree with), I think we can close this as we'll have done everything I can think of. Feel free to reopen if there are further ideas.

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

No branches or pull requests

6 participants