FIX: correctly handle large arcs by tacaswell · Pull Request #17564 · matplotlib/matplotlib · GitHub

The draw method of mpatches.Arc has two paths:

if the arc is "small" compared to its size in the rendered image
then render the whole arc and let clipping do it's thing
if the arc is "big" compared to its size on the screen then sort
out where the circle intersects the axes boundary and only draw
that part of it

This makes several changes to the Arc draw method:

only "squash" the angles in the "small" case is we handle it
in the "big" case via transforms
make sure that we don't re-order the angles while "squashing" them.

Adjusted the test image to use this failing case and to exercise both
code paths, but could be convinced to split it out into its own file.

closes #17547

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant

Any chance this fixes, or you understand it enough to fix, #7491?

Apparently we do need to squash the angles in both code paths.

I adjusted the tests and squashed this down to one commit.

On closer inspection, this test is not testing what I thought it was. Will work up a new test (and rebase).

This is ready for review again and very different that the original version. The new test should be more understandable as well.

I am still not sure that the threshold for switching to the "big path" logic is right, but it is possible I am confused about how it should work.

https://github.com/matplotlib/matplotlib/blob/ce9bade034cc09e3fb49d7d12fc83aa36c557cdb/lib/matplotlib/patches.py is I think the reference to look at when trying to understand this code.

If I force it to always run through the "small" code path this is how the new test fails:

so it is doing something, but I could not find a regime where it is visually different.

Looking at the paper, for a 4-spline, it said:

A one part in one thousand error would be one pixel on a screen circle of 14 inches at 73 DPI.

and since 8-spline is 1e-6 (or 1000 times smaller), it makes sense that we need a circle of such a large radius in order to see something with only about a pixel difference.

Not 100% on the tests, but the change seems right, individually.

Is this still the right trigger?

The second axis is trying to hit the "small" path which this does. These numbers could be made much smaller now that we have fixed the quadratic growth issue.

The draw method of mpatches.Arc has two paths: - if the arc is "small" compared to its size in the rendered image then render the whole arc and let clipping do it's thing - if the arc is "big" compared to its size on the screen then sort out where the circle intersects the axes boundary and only draw that part of it This makes several changes to the Arc draw method: - make sure that we keep angles in [0, 360) range - only go through the angle stretching code if we need to (to avoid numerical instability of angles not round-tripping with scale=1) - compute length, not offset from origin of width / height and use the correct transform. Previously we were effectively squaring the height and width Tests: - Adjusted an existing test image to use this failing case and to exercise both code paths. - Added a test function of ensuring we can draw a big arc in each quadrant

The previous commit removed a quadratic scaling when deciding which path we should go through. The quadratic scaling made us kick into the high-resolution code path too soon. We now no longer go through the high-resolution code path in this test which changes the curves slightly due to using different control points.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

probably needs a comment, IIRC you do this because theta1 and theta2 may not match anymore after stretching? (i.e. why can't you just always stretch)

can you instead first take everything mod 360 and then always stretch?

I tried that and it did not work, as 0%360 == 360%360. I guess we could keep track of if theta1 == theta2 up front and then add back 360 if they were? I don't think we want to special case theta1 == theta2 as input to mean "full circle" not "no arc".

why did arc_angles.png change?

Because this fixes a bug where we triggered the "big arc" path too soon in many cases (we were checking if r**2 > thresh not r > thresh) so that test is now all in the "small arc" regime.

Is that really r**2 vs r? I don't see where in the code...

get_transform scaled (1,1) to (width/2, height/2), so inputting (width, height) scales to (width**2/2, height**2/2), hence the switch to get_data_transform 'unsquares' the threshold.

@QuLogic beat me to posting, but link to where the transform is set up:

matplotlib/lib/matplotlib/patches.py

Lines 1398 to 1414 in 7f298e1

    
               def _recompute_transform(self): 
        
                   """ 
        
                   Notes 
        
                   ----- 
        
                   This cannot be called until after this has been added to an Axes, 
        
                   otherwise unit conversion will fail. This makes it very important to 
        
                   call the accessor method and not directly access the transformation 
        
                   member variable. 
        
                   """ 
        
                   center = (self.convert_xunits(self._center[0]), 
        
                             self.convert_yunits(self._center[1])) 
        
                   width = self.convert_xunits(self._width) 
        
                   height = self.convert_yunits(self._height) 
        
                   self._patch_transform = transforms.Affine2D() \ 
        
                       .scale(width * 0.5, height * 0.5) \ 
        
                       .rotate_deg(self.angle) \ 
        
                       .translate(*center)

I'm going to make an executive decision and self-merge this with one review.

it fixes a rather critical bug where no-arc is drawn
I think at least 2.5 of us now understand this code
this is the last thing standing between us, 3.2.2, and 3.3.0rc1

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.2.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 31e26cc73918ff20c44af8cb1d59256e67df996d

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #17564: FIX: correctly handle large arcs'

Push to a named branch :

git push YOURFORK v3.2.x:auto-backport-of-pr-17564-on-v3.2.x

Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR #17564 on branch v3.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

Merge pull request matplotlib#17564 from tacaswell/fix_big_arc FIX: big arc code path Conflicts: lib/matplotlib/patches.py - implicitly backport a change from matplotlib#15356 (from `- trans ` -> `+ trans.inverted()`)

Retrospectively approving :)

tacaswell added this to the v3.2.2 milestone Jun 4, 2020

tacaswell requested review from QuLogic, anntzer and dstansby June 4, 2020 04:21

QuLogic reviewed Jun 4, 2020

View reviewed changes

Comment thread lib/matplotlib/patches.py Outdated

Comment thread lib/matplotlib/tests/test_patches.py Outdated

Comment thread lib/matplotlib/tests/test_patches.py Outdated

Comment thread lib/matplotlib/tests/test_patches.py Outdated

tacaswell force-pushed the fix_big_arc branch 2 times, most recently from 1cca12a to 7f840f9 Compare June 5, 2020 21:38

tacaswell force-pushed the fix_big_arc branch 2 times, most recently from 0da5826 to 719a920 Compare June 8, 2020 18:00

tacaswell force-pushed the fix_big_arc branch 3 times, most recently from 620af63 to 03da86e Compare June 11, 2020 01:42

QuLogic reviewed Jun 12, 2020

View reviewed changes

Comment thread lib/matplotlib/patches.py Outdated

Comment thread lib/matplotlib/patches.py

QuLogic reviewed Jun 15, 2020

View reviewed changes

Comment thread lib/matplotlib/patches.py Outdated

tacaswell force-pushed the fix_big_arc branch 2 times, most recently from 95b85ae to d43b105 Compare June 15, 2020 23:10

QuLogic approved these changes Jun 16, 2020

View reviewed changes

tacaswell added 3 commits June 16, 2020 10:00

MNT: apply no-op exit for non-visible artists

7ae0c34

tacaswell force-pushed the fix_big_arc branch from 4aee88c to 7ae0c34 Compare June 16, 2020 14:07

QuLogic reviewed Jun 16, 2020

View reviewed changes

Comment thread lib/matplotlib/patches.py Outdated

DOC: remove duplicate word

d46c161

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

anntzer reviewed Jun 16, 2020

View reviewed changes

DOC: add more explanatory docstrings

0dfdfd6

tacaswell merged commit 31e26cc into matplotlib:master Jun 17, 2020

lumberbot-app Bot added the Still Needs Manual Backport label Jun 17, 2020

tacaswell deleted the fix_big_arc branch June 17, 2020 02:59

tacaswell mentioned this pull request Jun 17, 2020

Auto backport of pr 17564 on v3.2.x #17655

Merged

anntzer reviewed Jun 17, 2020

View reviewed changes

QuLogic removed the Still Needs Manual Backport label Oct 13, 2020

+                          # if we need to stretch the angles because we are distorted
+                          width != height
+                          # and we are not doing a full circle
+                          and not (theta1 != theta2 and theta1 % 360 == theta2 % 360)

		ax2.set_xlim(-25000, 18000)
		ax2.set_ylim(-20000, 6600)

FIX: correctly handle large arcs by tacaswell · Pull Request #17564 · matplotlib/matplotlib · GitHub

Repository navigation

FIX: correctly handle large arcs - #17564

FIX: correctly handle large arcs#17564tacaswell merged 5 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom tacaswell:fix_big_arctacaswell/matplotlib:fix_big_arcCopy head branch name to clipboard

Conversation

tacaswell commented Jun 4, 2020 • edited by QuLogic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QuLogic commented Jun 4, 2020

Uh oh!

tacaswell commented Jun 5, 2020

Uh oh!

tacaswell commented Jun 8, 2020

Uh oh!

tacaswell commented Jun 10, 2020

Uh oh!

tacaswell commented Jun 11, 2020

Uh oh!

Uh oh!

Uh oh!

tacaswell commented Jun 12, 2020

Uh oh!

Uh oh!

tacaswell commented Jun 15, 2020

Uh oh!

QuLogic commented Jun 16, 2020

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QuLogic Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

tacaswell Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anntzer Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

tacaswell Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

anntzer commented Jun 16, 2020

Uh oh!

tacaswell commented Jun 16, 2020

Uh oh!

anntzer commented Jun 16, 2020

Uh oh!

QuLogic commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Jun 16, 2020

Uh oh!

tacaswell commented Jun 17, 2020

Uh oh!

lumberbot-app Bot commented Jun 17, 2020

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FIX: correctly handle large arcs#17564
tacaswell merged 5 commits into
matplotlib:masterfrom
tacaswell:fix_big_arc

tacaswell commented Jun 4, 2020 •
edited by QuLogic
Loading

anntzer Jun 16, 2020 •
edited
Loading

QuLogic commented Jun 16, 2020 •
edited
Loading