-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Vectorize Arc.draw. #14694
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
Vectorize Arc.draw. #14694
Conversation
Went with the middle suggestion, which is certainly not worse than row_stack :) |
d9855d3
to
f7be415
Compare
According to codecov, |
... by replacing generators into functions working on numpy arrays. All modified functions are nested and thus not publically accessible. Also remove handling the tangential case as the angles are immediately stuffed in a set() so duplicate angles don't matter.
Added a test. Removed specific handling of tangential case because the results are immediately stuffed into a set(), so duplicate angles don't matter (and because I didn't want to write a test for it...). |
Hm, tangential is still a special case. Just removing the special-cased code does not guarantee that the case is handled correctly by the generic code. A test for that would be good nevertheless. |
I was going to argue that an additional test for the tangential case was unneeded when I noticed something funny: the "large arc, high accuracy" case (which is what we're talking about here) is actually broken! Consider from matplotlib import pyplot as plt, patches as mpatches
ax = plt.figure().add_subplot(111)
# Large arcs that crosses the axes view limits close to x=0.5.
ax.add_patch(mpatches.Arc((-10, 0.5), 21, 21, theta1=-10, theta2=10, edgecolor="b"))
ax.add_patch(mpatches.Arc((-100, 0.5), 201, 201, theta1=-10, theta2=10, edgecolor="r"))
plt.show() The first arc does not trigger the "large arc, high accuracy" case and is drawn, but the second one isn't. This is a "regression" from #9661 (@QuLogic), although that PR fixed a separate logic error in the code that caused partial arcs to be drawn as full arcs and without using the "large arc, high accuracy" code... Going back further, it appears that the "large arc, high accuracy" case had been effectively lost since b7479ef (10 years ago). It's a bit sad, of course, because this entire thing was apparently written (in 7a82ea8 -- 2007) as support for the Mars Phoenix mission (https://matplotlib.org/tutorials/introductory/sample_plots.html?highlight=phoenix#ellipses). On the other hand, given the amount of time for which this has been broken, and given that no one ever complained about the accuracy loss, I think we can perhaps just drop the accuracy claim and draw the large arc with a standard Bézier approximation. In fact, I would say that a better approach for drawing arcs may be to add an "ARC" code to Path objects (similar to LINETO/CURVE3/CURVE4) and let the renderers deal with it: at least cairo (https://www.cairographics.org/manual/cairo-Paths.html#cairo-arc), SVG (https://developer.mozilla.org/en-US/docs/Web/SVG/Element/circle) and postscript (http://www.physics.emory.edu/faculty/weeks/graphics/howtops2.html) have primitives for it, and it seems likely that Agg and pdf also have one. |
OK, we have two approvals, but now I'm not sure if you're saying we should fix more things, or merge it anyway? |
I would say this doesn't make things worse and improves the testing a bit, so this can be merged unless @timhoffm insists on more tests (let's wait for his opinion), but things are actually pretty broken to start with and probably I'll do bigger surgery later. |
I'll merge, and we can add more tests if @timhoffm still thinks we should. |
We'll let's hope the tests are sufficient. Not going to spend time on this for now. |
Vectorize Arc.draw.
... by replacing generators into functions working on numpy arrays.
All modified functions are nested and thus not publically accessible.
PR Summary
PR Checklist