Correct theta values when drawing a non-circular arc by dstansby · Pull Request #8047 · matplotlib/matplotlib

dstansby · 2017-02-08T13:27:15Z

Fixes #8046. I'm not sure if this is the right place to do the correction, so a second opinion would be good. I've checked this works with a full range of angles.

tacaswell · 2017-02-08T15:22:41Z

lib/matplotlib/tests/test_axes.py

    ax.plot([1, 2, 3])


+@image_comparison(baseline_images=['arc_angles'])


add remove_text=True and style='default' to the decorator to get the new defaults + drop all of the text (which makes the test more stable).

tacaswell · 2017-02-08T15:24:38Z

This will need an entry in whats new or api changes. The paranoid case here is that users have worked around this bug and we are now silently changing things under them so it needs to be documented.

tacaswell · 2017-02-08T15:28:20Z

This looks like the correct place to do this to me. Ellipse is implemented by holding transforms to re-scale a circular path so pushing this down into Path.arc is not right and Ellipse is for full coverage so pushing this into the base-class is not right either.

dstansby · 2017-02-10T20:00:09Z

Does anyone know why the documentation build is failing with:

/Users/dstansby/matplotlib/doc/examples/units/ellipse_with_units.rst:8: WARNING: Exception occurred in plotting ellipse_with_units
 from /Users/dstansby/matplotlib/doc/mpl_examples/units/ellipse_with_units.py:
Traceback (most recent call last):
  File "/Users/dstansby/matplotlib/lib/matplotlib/sphinxext/plot_directive.py", line 525, in run_code
    six.exec_(code, ns)
  File "<string>", line 55, in <module>
  File "/Users/dstansby/matplotlib/lib/matplotlib/patches.py", line 1565, in __init__
    theta1 = theta_stretch(theta1, width / height)
TypeError: unsupported operand type(s) for /: 'TaggedValue' and 'TaggedValue'

afvincent · 2017-02-10T21:04:14Z

As far as I understand, it seems like “width” and “height” are cm instances from this script and that the class used under the hood (TaggedValue) does not implement the division operation.

dstansby · 2017-02-10T21:09:28Z

Would anyone object to me just getting rid of http://matplotlib.org/examples/units/ellipse_with_units.html ? It doesn't seem to be showing any unique capabilities of units, and the 'comparison' just looks the same to me.

(and does it matter if my new code breaks drawing an ellipse with units?)

tacaswell · 2017-02-10T21:22:33Z

Yes and yes (although, it seems like the problem here is the exact unit implementation, not generally).

The unit support is an under-understood, but critical feature of mpl (JPL relies on it).

dstansby · 2017-02-10T22:00:13Z

As far as I can tell:

The correct way to convert from units to a number is call self.convert_xunits(width)
However this is just returning width unchanged as units
This is because there is no axis when __init__ is called on Arc, so the units are returned unchanged

so all this means there's no way of converting units within the __init__ bit of Arc, so the conversion and angle correction needs to happen somewhere else.

dstansby · 2017-02-10T22:27:46Z

That should fix it, I've moved the new calculation into the draw() method.

QuLogic

LGTM mostly. Just a small tweak to the test.

QuLogic · 2017-02-23T03:07:37Z

lib/matplotlib/tests/test_axes.py

+        theta2 = i * 360 / 9
+        theta1 = theta2 - 45
+        ax.add_patch(patches.Arc(centre, w, h, theta1=theta1, theta2=theta2))
+        # Straight line should intersect end of arc


Could you also add the start of the arc as well? Start -> origin -> end should work.

QuLogic · 2017-02-23T03:07:50Z

lib/matplotlib/tests/test_axes.py

+    centre = (0.2, 0.5)
+
+    fig, axs = plt.subplots(3, 3)
+    for i, ax in enumerate(np.ravel(axs)):


enumerate(axs.flat)

QuLogic · 2017-02-23T03:09:52Z

lib/matplotlib/tests/test_axes.py

+    for i, ax in enumerate(np.ravel(axs)):
+        theta2 = i * 360 / 9
+        theta1 = theta2 - 45
+        ax.add_patch(patches.Arc(centre, w, h, theta1=theta1, theta2=theta2))


Can you also add a full Ellipse patch with the same parameters but maybe half linewidth or alpha just to show that the arc is in a consistent place?

dstansby · 2017-02-27T15:37:20Z

Thanks for the extra ideas for the test @QuLogic - once this has been reviewed I can squash everything down to a single commit.

QuLogic · 2017-02-28T01:54:34Z

Is there anything backend-specific here? Do we need the SVG/PDF files?

dstansby · 2017-02-28T14:38:19Z

Oh yes, I forgot that those were optional. I don't think there's anything backend specific, shall I just have a .png image test?

QuLogic · 2017-03-02T05:15:14Z

Yes, I think we can drop the other ones and you can squash this.

Make arcs work with units Add api change not for elliptical notes Add background ellipse and extra line to test Remove pdf and svg tests for arc angles

dstansby · 2017-03-02T21:48:31Z

Great, that should be all nicely squashed up now.

QuLogic

One more minor thing.

QuLogic · 2017-03-02T21:50:04Z

doc/api/api_changes/2017-02-10-DS_elliptical_arc_angle.rst

+The `matplotlib.patches.Arc` patch is now correctly drawn between the given
+angles.
+
+Previously a circular are was drawn and then stretched into an ellipse,


tacaswell added this to the 2.1 (next point release) milestone Feb 8, 2017

tacaswell reviewed Feb 8, 2017

View reviewed changes

dstansby force-pushed the arc-theta branch from 00891c9 to 29470f9 Compare February 8, 2017 17:59

dstansby changed the title ~~Correct theta values when drawing a non-circular ellipse~~ Correct theta values when drawing a non-circular arc Feb 9, 2017

dstansby added the status: needs review label Feb 14, 2017

QuLogic reviewed Feb 23, 2017

View reviewed changes

Correct theta values when drawing a non-circular ellipse

4bc50da

Make arcs work with units Add api change not for elliptical notes Add background ellipse and extra line to test Remove pdf and svg tests for arc angles

dstansby force-pushed the arc-theta branch from 785a8f9 to 4bc50da Compare March 2, 2017 21:48

QuLogic reviewed Mar 2, 2017

View reviewed changes

Fix small typo in api change

0376d83

QuLogic approved these changes Mar 9, 2017

View reviewed changes

QuLogic changed the title ~~Correct theta values when drawing a non-circular arc~~ [MRG+1] Correct theta values when drawing a non-circular arc Mar 9, 2017

NelleV approved these changes Mar 10, 2017

View reviewed changes

NelleV merged commit beeba66 into matplotlib:master Mar 10, 2017

QuLogic changed the title ~~[MRG+1] Correct theta values when drawing a non-circular arc~~ Correct theta values when drawing a non-circular arc Mar 11, 2017

QuLogic removed the status: needs review label Mar 11, 2017

dstansby deleted the arc-theta branch April 2, 2017 16:44

ImportanceOfBeingErnest mentioned this pull request May 19, 2018

Arc patch ignoring theta1/theta2 when added to Axes via PatchCollection #11266

Closed

anntzer mentioned this pull request Jun 2, 2020

Arcs with large radii in small #17547

Closed

kiwifb mentioned this pull request Dec 25, 2017

Update matplotlib to 2.1.0 sagemath/sage#23696

Closed

		ax.plot([1, 2, 3])


		@image_comparison(baseline_images=['arc_angles'])

Uh oh!

Conversation

dstansby commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Feb 8, 2017

Uh oh!

tacaswell commented Feb 8, 2017

Uh oh!

dstansby commented Feb 10, 2017

Uh oh!

afvincent commented Feb 10, 2017

Uh oh!

dstansby commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Feb 10, 2017

Uh oh!

dstansby commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstansby commented Feb 10, 2017

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

QuLogic Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

QuLogic Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

dstansby commented Feb 27, 2017

Uh oh!

QuLogic commented Feb 28, 2017

Uh oh!

dstansby commented Feb 28, 2017

Uh oh!

QuLogic commented Mar 2, 2017

Uh oh!

dstansby commented Mar 2, 2017

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

dstansby Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dstansby commented Feb 8, 2017 •

edited

Loading

dstansby commented Feb 10, 2017 •

edited

Loading

dstansby commented Feb 10, 2017 •

edited

Loading