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 isFlake 8 compliant

tacaswell added this to thev3.2.2 milestone

Jun 4, 2020

tacaswell requested review fromQuLogic,anntzer anddstansby

June 4, 2020 04:21

QuLogic reviewed

Jun 4, 2020

View reviewed changes

lib/matplotlib/patches.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_patches.py OutdatedShow resolvedHide resolved

Copy link

Member

QuLogic commentedJun 4, 2020

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

tacaswell force-pushed thefix_big_arc branch 2 times, most recently from1cca12a to7f840f9Compare

June 5, 2020 21:38

Copy link

MemberAuthor

tacaswell commentedJun 5, 2020

Apparently wedo need to squash the angles in both code paths.

tacaswell force-pushed thefix_big_arc branch 2 times, most recently from0da5826 to719a920Compare

June 8, 2020 18:00

Copy link

MemberAuthor

tacaswell commentedJun 8, 2020

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

Copy link

MemberAuthor

tacaswell commentedJun 10, 2020

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

tacaswell force-pushed thefix_big_arc branch 3 times, most recently from620af63 to03da86eCompare

June 11, 2020 01:42

Copy link

MemberAuthor

tacaswell commentedJun 11, 2020

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.

QuLogic reviewed

Jun 12, 2020

View reviewed changes

lib/matplotlib/patches.py OutdatedShow resolvedHide resolved

lib/matplotlib/patches.pyShow resolvedHide resolved

Copy link

MemberAuthor

tacaswell commentedJun 12, 2020

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.

QuLogic reviewed

Jun 15, 2020

View reviewed changes

lib/matplotlib/patches.py OutdatedShow resolvedHide resolved

tacaswell force-pushed thefix_big_arc branch 2 times, most recently from95b85ae tod43b105Compare

June 15, 2020 23:10

Copy link

MemberAuthor

tacaswell commentedJun 15, 2020

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.

Copy link

Member

QuLogic commentedJun 16, 2020

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.

QuLogic approved these changes

Jun 16, 2020

View reviewed changes

Copy link

Member

QuLogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

lib/matplotlib/patches.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_patches.py

Comment on lines +520 to +521

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is this still the right trigger?

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

tacaswell added3 commits

June 16, 2020 10:00

FIX: correctly handle large arcs

98ab2a0

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 itThis 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 widthTests: - 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

TST: No longer go through high-resolution code path in test

a302b5a

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

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

7ae0c34

tacaswell force-pushed thefix_big_arc branch from4aee88c to7ae0c34Compare

June 16, 2020 14:07

QuLogic reviewed

Jun 16, 2020

View reviewed changes

lib/matplotlib/patches.py OutdatedShow resolvedHide resolved

DOC: remove duplicate word

d46c161

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

anntzer reviewed

Jun 16, 2020

View reviewed changes

lib/matplotlib/patches.py

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

Copy link

Contributor

anntzerJun 16, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

anntzer commentedJun 16, 2020

why did arc_angles.png change?

Copy link

MemberAuthor

tacaswell commentedJun 16, 2020

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

Copy link

Contributor

anntzer commentedJun 16, 2020

Is that reallyr**2 vsr? I don't see where in the code...

Copy link

Member

QuLogic commentedJun 16, 2020•
edited
Loading

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 toget_data_transform 'unsquares' the threshold.

Copy link

MemberAuthor

tacaswell commentedJun 16, 2020

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

matplotlib/lib/matplotlib/patches.py

Lines 1398 to 1414 in7f298e1

	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(width0.5,height0.5) \
	.rotate_deg(self.angle) \
	.translate(*center)

DOC: add more explanatory docstrings

0dfdfd6

Copy link

MemberAuthor

tacaswell commentedJun 17, 2020

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

tacaswell merged commit31e26cc intomatplotlib:master

Jun 17, 2020

Copy link

lumberbot-appbot commentedJun 17, 2020

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 tosuggest an improvement.

lumberbot-appbot added the status: needs manual backport label

Jun 17, 2020

tacaswell deleted the fix_big_arc branch

June 17, 2020 02:59

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request

Jun 17, 2020

Backport PRmatplotlib#17564: FIX: correctly handle large arcs

d67550c

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

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

Copy link

Contributor

anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Retrospectively approving :)

QuLogic removed the status: needs manual backport label

Oct 13, 2020

Labels

None yet

3 participants

Movatterモバイル変換

Uh oh!

FIX: correctly handle large arcs#17564

FIX: correctly handle large arcs#17564

Uh oh!

Conversation

tacaswell commentedJun 4, 2020• edited by QuLogicLoading 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 commentedJun 4, 2020

Uh oh!

tacaswell commentedJun 5, 2020

Uh oh!

tacaswell commentedJun 8, 2020

Uh oh!

tacaswell commentedJun 10, 2020

Uh oh!

tacaswell commentedJun 11, 2020

Uh oh!

Uh oh!

Uh oh!

tacaswell commentedJun 12, 2020

Uh oh!

Uh oh!

tacaswell commentedJun 15, 2020

Uh oh!

QuLogic commentedJun 16, 2020

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QuLogicJun 16, 2020

Choose a reason for hiding this comment

Uh oh!

tacaswellJun 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anntzerJun 16, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzerJun 17, 2020

Choose a reason for hiding this comment

Uh oh!

tacaswellJun 17, 2020

Choose a reason for hiding this comment

Uh oh!

anntzer commentedJun 16, 2020

Uh oh!

tacaswell commentedJun 16, 2020

Uh oh!

anntzer commentedJun 16, 2020

Uh oh!

QuLogic commentedJun 16, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

tacaswell commentedJun 16, 2020

Uh oh!

tacaswell commentedJun 17, 2020

Uh oh!

lumberbot-appbot commentedJun 17, 2020

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tacaswell commentedJun 4, 2020•
edited by QuLogic
Loading

anntzerJun 16, 2020•
edited
Loading

QuLogic commentedJun 16, 2020•
edited
Loading