Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jklymak merged 1 commit intomatplotlib:masterfromanntzer:arcdraw
Oct 1, 2019
Merged

Vectorize Arc.draw.#14694

jklymak merged 1 commit intomatplotlib:masterfromanntzer:arcdraw
Oct 1, 2019

Conversation

anntzer
Copy link
Contributor

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

@anntzer
Copy link
ContributorAuthor

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

@anntzeranntzerforce-pushed thearcdraw branch 2 times, most recently fromd9855d3 tof7be415CompareJuly 17, 2019 17:30
@QuLogic
Copy link
Member

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 immediatelystuffed in a set() so duplicate angles don't matter.
@anntzer
Copy link
ContributorAuthor

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
ContributorAuthor

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

frommatplotlibimportpyplotasplt,patchesasmpatchesax=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 sinceb7479ef (10 years ago).

It's a bit sad, of course, because this entire thing was apparently written (in7a82ea8 -- 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

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
ContributorAuthor

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.

@tacaswelltacaswell added this to thev3.3.0 milestoneAug 24, 2019
@jklymak
Copy link
Member

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

@jklymakjklymak merged commit85efd95 intomatplotlib:masterOct 1, 2019
@anntzeranntzer deleted the arcdraw branchOctober 1, 2019 17:02
@anntzeranntzer mentioned this pull requestOct 1, 2019
6 tasks
@timhoffm
Copy link
Member

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

@tacaswelltacaswell modified the milestones:v3.3.0,v3.2.2Jun 17, 2020
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull requestJun 17, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.2.2
Development

Successfully merging this pull request may close these issues.

5 participants
@anntzer@QuLogic@timhoffm@jklymak@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp