Skip to content
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

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Vectorize Arc.draw. #14694

merged 1 commit into from
Oct 1, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 4, 2019

... by replacing generators into functions working on numpy arrays.

All modified functions are nested and thus not publically accessible.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

lib/matplotlib/patches.py Outdated Show resolved Hide resolved
lib/matplotlib/patches.py Outdated Show resolved Hide resolved
@anntzer
Copy link
Contributor Author

anntzer commented Jul 4, 2019

Went with the middle suggestion, which is certainly not worse than row_stack :)
Also replaced np.abs by abs.

lib/matplotlib/patches.py Outdated Show resolved Hide resolved
@anntzer anntzer force-pushed the arcdraw branch 2 times, most recently from d9855d3 to f7be415 Compare July 17, 2019 17:30
@QuLogic
Copy link
Member

QuLogic commented Jul 18, 2019

According to codecov, line_circle_intersect is always returning the empty array. I'm not sure how to add a test to trigger the other paths.

... 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.
@anntzer
Copy link
Contributor Author

anntzer commented Jul 18, 2019

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...).

@timhoffm
Copy link
Member

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.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 18, 2019

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.

@QuLogic
Copy link
Member

QuLogic commented Jul 25, 2019

OK, we have two approvals, but now I'm not sure if you're saying we should fix more things, or merge it anyway?

@anntzer
Copy link
Contributor Author

anntzer commented Jul 25, 2019

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.

@tacaswell tacaswell added this to the v3.3.0 milestone Aug 24, 2019
@jklymak
Copy link
Member

jklymak commented Oct 1, 2019

I'll merge, and we can add more tests if @timhoffm still thinks we should.

@jklymak jklymak merged commit 85efd95 into matplotlib:master Oct 1, 2019
@anntzer anntzer deleted the arcdraw branch October 1, 2019 17:02
@anntzer anntzer mentioned this pull request Oct 1, 2019
6 tasks
@timhoffm
Copy link
Member

timhoffm commented Oct 3, 2019

We'll let's hope the tests are sufficient. Not going to spend time on this for now.

@tacaswell tacaswell modified the milestones: v3.3.0, v3.2.2 Jun 17, 2020
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jun 17, 2020
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

Successfully merging this pull request may close these issues.

5 participants