Repository navigation
Optimise Axes.add_patch to avoid python iteration over bezier segments - #28657
tpgillam wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
|
Unfortunately this doesn't pass many tests so I suspect your optimization is returning very different results than the usual code. It may be helpful if you make a test example that is very slow for you so we can understand clearly what you are trying to achieve and then folks may have suggestions. |
Sorry, something went wrong.
|
Indeed, I saw that! I'd like to be able to see what the differences are locally; but it seems that I can't even run one by passing a specific path to pytest. (It hangs with zero output and strace tells me it's just in an infinite loop of wait4 and pselect6 calls - never come across this before!) My suspicion is that the difference might be to do with a simple consideration of control points vs extremal values on the curve; but very hard to say without being able to see the output. |
Sorry, something went wrong.
|
Here is a silly example, but it's minimal whilst demonstrating the issue: import timeit
import numpy
from matplotlib import pyplot
from matplotlib.axes import Axes
from matplotlib.collections import PatchCollection
from matplotlib.patches import Polygon
points = numpy.random.rand(10000, 2)
patch = Polygon(points)
_, axes1 = pyplot.subplots()
assert isinstance(axes1, Axes)
t1 = timeit.timeit(lambda: axes1.add_patch(patch), number=1)
_, axes2 = pyplot.subplots()
assert isinstance(axes2, Axes)
t2 = timeit.timeit(
lambda: axes2.add_collection(PatchCollection([patch], match_original=True)),
number=1,
)
print(t1)
print(t2)0.10465810797177255 0.000516503001563251 The difference in this case is nearly a factor of 200x on my machine. In a more realistic case where also considering figure construction then it's less significant, but still noticeable. Running the first one through the profiler shows the time being spent in _update_patch_limits. 240329 function calls (240324 primitive calls) in 0.148 seconds
Ordered by: cumulative time
List reduced from 138 to 40 due to restriction <40>
ncalls tottime percall cumtime percall filename:lineno(function)
1 0.000 0.000 0.148 0.148 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/axes/_base.py:2406(add_patch)
1 0.016 0.016 0.148 0.148 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/axes/_base.py:2419(_update_patch_limits)
10002 0.010 0.000 0.066 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/path.py:413(iter_bezier)
10001 0.029 0.000 0.052 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/bezier.py:208(__call__)
10001 0.033 0.000 0.047 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/bezier.py:199(__init__)
20002 0.020 0.000 0.020 0.000 {method 'outer' of 'numpy.ufunc' objects}
10001 0.008 0.000 0.011 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/bezier.py:203(<listcomp>)
10001 0.005 0.000 0.010 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/bezier.py:283(axis_aligned_extrema)
30003 0.009 0.000 0.009 0.000 {built-in method numpy.array}
1 0.001 0.001 0.004 0.004 /somewhere/.venv/lib/python3.11/site-packages/numpy/core/shape_base.py:219(vstack)
10002 0.003 0.000 0.004 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/path.py:347(iter_segments)
10001 0.003 0.000 0.003 0.000 {built-in method numpy.arange}
20008 0.003 0.000 0.003 0.000 {built-in method numpy.asarray}
60003 0.002 0.000 0.002 0.000 {built-in method math.factorial}
1 0.002 0.002 0.002 0.002 /somewhere/.venv/lib/python3.11/site-packages/numpy/core/shape_base.py:81(atleast_2d)
20003 0.001 0.000 0.001 0.000 {method 'append' of 'list' objects}
10001 0.001 0.000 0.001 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/bezier.py:242(degree)
10002 0.000 0.000 0.000 0.000 {built-in method numpy.asanyarray}
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/path.py:470(cleaned)
1 0.000 0.000 0.000 0.000 {built-in method matplotlib._path.cleanup_path}
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/axes/_base.py:2521(update_datalim)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:931(update_from_data_xy)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:859(update_from_path)
1 0.000 0.000 0.000 0.000 {built-in method matplotlib._path.update_path_extents}
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/artist.py:769(set_clip_path)
2 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/patches.py:306(get_transform)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:1428(__sub__)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/patches.py:790(get_patch_transform)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/numpy/core/shape_base.py:215(_vhstack_dispatcher)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/numpy/core/shape_base.py:207(_arrays_for_stack_dispatcher)
3/2 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:2394(__eq__)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:1720(__eq__)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:2172(__eq__)
3/2 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:1787(__eq__)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/path.py:99(__init__)
4 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:1350(__add__)
4 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:2517(composite_transform_factory)
2 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:2401(_iter_break_from_left_to_right)
3 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/transforms.py:2358(__init__)
1 0.000 0.000 0.000 0.000 /somewhere/.venv/lib/python3.11/site-packages/matplotlib/patches.py:924(get_bbox)
In our real use we're somewhat performance-sensitive when drawing fairly complex patches. So removing this bottleneck by pushing the iteration down to C++ would be helpful. My hope was that adding a PatchCollection would have the same data-limit-adjusting properties as adding the patches individually; and if this were true then my aim with this PR was to mimic this within add_patch. |
Sorry, something went wrong.
|
The actions have an "Upload Artifact " step with a link to the artifacts. No idea why your pytest is not working. I would make sure it is running from the same python as your env. |
Sorry, something went wrong.
Finally got a python stack trace after interrupting pytest --help that shows it hanging in pyvirtualdisplay.abstractdisplay::_wait_for_pipe_text. I'm on a work machine running WSL at the moment, seems plausible that this won't be helping. |
Sorry, something went wrong.
|
You can use blame to see that this code came in relatively recently: #19124 and #24177. At the least if you remove this code I'd expect test_bezier_autoscale and test_small_autoscale to definitely fail. If you are to remove it, then you'd want an argument why the fixes in those PRs are not worth the performance hit. Unfortunately, you can usually only have two of easy, quick, or good. We are probably going to err on the side of easy & good, rather than easy & quick. Of course you also seem to have removed extent finding all together as some of the changes are not trivial in the rest of the test suite. |
Sorry, something went wrong.
|
My underlying assumption here was that the limit adjustment for a collection of one patch would be the same as for that one patch alone. I think my understanding from these failures (and a few inferences about the code) is that this assumption is false, although without the ability to iterate on the tests locally I'm not 100% sure. Anyway, I'll close this PR. I'm not advocating for changing behaviour; it just appeared at a glance that this might be behaviour preserving, but never mind! |
Sorry, something went wrong.
|
If you have very large paths with lots of bezier curves, it might be worth turning off autoscaling and handling setting the limits your self. |
Sorry, something went wrong.
Thanks for the suggestion. Unfortunately calling axes.autoscale(False) first doesn't make any difference. (More generally, I can't see any user-configurable way to prevent the call to _iter_bezier when calling add_patch. The data limits are always updated, regardless of the autoscale settings on the axes themselves.) |
Sorry, something went wrong.
|
hmm, looking at the code it looks like we always compute the data limits and only sometimes update the view limits. I can see a case for making "update the data limits" lazy and computing it on the demand, but I do not think that would be a small project to do. |
Sorry, something went wrong.
|
Sorry for pointing you down a dead-end. |
Sorry, something went wrong.
PR summary
This aims to optimise calls to _AxesBase.add_patch, where the patch might have a path with many bezier segments. This is done by avoiding a long loop in python, and instead delegating to BBox.update_from_path, which is used when updating limits from other artists.
This provides a ~5x speedup for a call to _AxesBase.add_patch in some of our internal code, with complicated patches with hundreds of bezier segments.
Implementation notes
The implementation is effectively the same as that in _AxesBase._update_line_limits also in this class. It performs the iteration in C++ (in _path.h::update_path_extents).
This is the same approach as used for collections of patches; i.e. the rescaling behaviour is now as performant as:
PR checklist
WIP: trying but so far have failed to get the test suite running locally. I followed steps to create necessary venv and build, but running pytest just hangs indefinitely without consuming any resources. I can however import the local build of matplotlib and perform basic tasks with it by hand.