plot Arc of circle and ellipse

[videlec]

Implementation of arc plot

This just wraps the matplotlib functionnality Arc http://matplotlib.sourceforge.net/api/artist_api.html#matplotlib.patches.Arc

This can be reviewd together with the ticket #9203 for plotting ellipse. On the latter ticket there is an attached worksheet that does some nice pictures and performs tests for the get_minmax_data function.

Rk: there is not yet fill capabilities (due to matplotlib).

CC: @sagetrac-sage-combinat @kcrisman @jasongrout

Component: graphics

Keywords: plot, arc, ellipse, circle

Author: Vincent Delecroix

Reviewer: Karl-Dieter Crisman

Merged: sage-4.6.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/9076

[JohnCremona]

comment:4

I cannot review this as I know nothing about matplotlib (or graphics) and only learned 2 days ago how to draw a circle -- but what I really wanted was semicircles, so I will be happy when this is ready!

[kcrisman]

comment:5

This patch is corrupt (in a sense) and has two copies of each changeset. The second changeset seems to be a little more complete, so perhaps that is the correct one? This will certainly be very easy to fix.

[videlec]

comment:6

I fixed the patch (and I created another one #9203 for plotting ellipse).

[videlec]

Description changed:

--- 
+++ 
@@ -3,4 +3,6 @@
 This just wraps the matplotlib functionnality Arc
 http://matplotlib.sourceforge.net/api/artist_api.html#matplotlib.patches.Arc

-there is not yet fill capabilities (due to matplotlib).
+This can be reviewd together with the ticket #9203 for plotting ellipse.
+
+Rk: there is not yet fill capabilities (due to matplotlib).

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:7

In general, I like this. Here are some specific suggestions to make it tip-top.

• line 26 - "an arc", not just "arc"

• There should be some examples for the class def, since that is in reference

• allowed options docs has circle a couple times rather than arc

• The 'repr' method should have an explanation of radius, maybe even to the point of checking whether it is a circle or an ellipse (and then returning an appropriate string)

• Should be a plot3d method, or open a ticket to add one - though really one could just ape circle.py with x+rcos(tdt) and y+rsin(tdt) changed appropriately - and if not, then we should test the NotImplementedError raised in the base class.

• in arc(), we should have a few changes. First, a few more of the options should be shown in the examples. Second, I think a 2-uple is a 2-tuple (?). Third, clearly the radii don't need to be floats, thought presumably they should be nonnegative. Next, the sector should be clearly pointed out to be a 2-tuple in the input. Finally, of the two examples shown, the red one isn't! It would be good to give an explicit example of how angle (as opposed to sector) works, perhaps similarly to the one in get_min_max_data. One should test the NotImplementedError at the end (under TESTS::, I guess)

[videlec]

Description changed:

--- 
+++ 
@@ -3,6 +3,6 @@
 This just wraps the matplotlib functionnality Arc
 http://matplotlib.sourceforge.net/api/artist_api.html#matplotlib.patches.Arc

-This can be reviewd together with the ticket #9203 for plotting ellipse.
+This can be reviewd together with the ticket #9203 for plotting ellipse. On the latter ticket there is an attached worksheet that does some nice pictures and performs tests for the get_minmax_data function.

 Rk: there is not yet fill capabilities (due to matplotlib).

[videlec]

comment:10

I get two problems in arc/ellipse:

• there is an uncompatiblity of option names between disk and arc/ellipse. The function disk uses angle as the angle of the sector to draw as arc/ellipse uses angle but arc uses sector. Moreover the word angle is reserved for the angle between the horizontal and the angle of the first axis

  • I did not find any function that allows to make some calculus modulo 2pi. I created a dummy function mod2pi that does the reduction.

[jasongrout]

comment:11

Replying to @videlec:

• I did not find any function that allows to make some calculus modulo 2pi. I created a dummy function mod2pi that does the reduction.

sage: math.fmod(10,2*pi)
3.7168146928204138

See http://docs.python.org/library/math.html#math.fmod

[kcrisman]

comment:12

I am sorry for the following laundry list. I want to reiterate that in general this is very nice, but the hope is that this will be the best possible! Thanks for your patience.

• I think that the current algorithm for the bounding box is not the same as that in the worksheet, which seems to be pretty good. The ellipse part works; I did find one example where the arc one didn't quite work:

my_arc=arc((1,-2),2,5,-pi/5,(3*pi/2,pi/4),thickness=2)
my_d=my_arc[0].get_minmax_data()
my_arc+polygon([(my_d['xmin'],my_d['ymin']),
          (my_d['xmin'],my_d['ymax']),
          (my_d['xmax'],my_d['ymax']),
          (my_d['xmax'],my_d['ymin'])],rgbcolor='red')

my_arc=arc((0,0),2,5,-pi/5,(3*pi/2,pi/4),thickness=2)
my_xmin,my_ymin,my_xmax,my_ymax=arc_bounding_box(2,5,-pi/5,3*pi/2,pi/4)
my_arc+polygon([(my_xmin,my_ymin),
          (my_xmin,my_ymax),
          (my_xmax,my_ymax),
          (my_xmax,my_ymin)],rgbcolor='red')

Maybe the part of the algorithm that checks whether to use the sector endpoints is too lax?

• The worksheet is REALLY COOL, by the way; I strongly encourage you to add more commentary, turn the graphics into interacts, and advertise it as a great way to use Sage to demonstrate basic uses of parametric calculus!

• With respect to fmod - yes, this makes sense to use. (Otherwise we'd have to document the dummy function!)

• I'd also point out that since you already imported pi from math, there should be no need to float it (? I think?); this happens a lot in this code.

----------------------------------------------------------------------
| Sage Version 4.4.2, Release Date: 2010-05-19                       |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
sage: from math import pi
sage: pi
3.1415926535897931
sage: float(pi)
3.1415926535897931

• On a related point, the constructor should be as minimal as possible; why not wait to float the radii etc. until _render_on_subplot_() and get_minmax_data()? That would also make the representation string much easier on the eyes (and intelligible).

• Space missing in the sage/plot/plot.py reference

• My sense is that the point of having @rename_keyword(color='rgbcolor') is so that

sage: arc((0,0), 2, 1, 0, (0,pi/2), color="red") 

would work, which is more natural to the user (though I may be misunderstanding the documentation for sage.plot.misc.rename_keyword). I realize that circle.py doesn't really take advantage of this... and really an rgbcolor should be a 3-tuple, morally speaking... That example doesn't have linestyle='--', incidentally.

• Center should still be 2-tuple, not 2-uple

• One may want to point out that alpha refers to transparency.

• Minh will surely point out that we wish uniformity on 2-d, 2D, 2-D, or whatever. I think the current convention is 2-D, though some docs have 2d or 2D. But at any rate within a file it should be consistent.

• Again, in the docs for arc(), the radii do NOT have to be floats.

• should there be a check for negative radii, like in the patch for ellipse.py? arc((1,1),1,-1,pi,(pi/2,pi)) is logically correct but maybe would allow bugs in code of people using arc to slip through.

But again all of this takes nothing away from what is in general a great contribution.

[videlec]

Attachment: trac_9076-arc.patch.gz

[videlec]

comment:13

• I think that the current algorithm for the bounding box is not the same as that in the worksheet, which seems to be pretty good. The ellipse part works; I did find one example where the arc one didn't quite work:

There was a mistake... this now works on a lot (all?) examples.

• The worksheet is REALLY COOL, by the way; I strongly encourage you to add more commentary, turn the graphics into interacts, and advertise it as a great way to use Sage to demonstrate basic uses of parametric calculus!

I tried to enhanced the worksheet. I do not know what I can do more.

• On a related point, the constructor should be as minimal as possible; why not wait to float the radii etc. until _render_on_subplot_() and get_minmax_data()? That would also make the representation string much easier on the eyes (and intelligible).

Here, I disagree because all plotting functions convert in the constructor. I do not know why, but for consistency I find it better to do this way. An other argument for this option is to get an error as soon as possible if the constructor is called with wrong argument.

I corrected all the other points in the way you suggested.

[kcrisman]

comment:14

Okay, the constructor is not so important anyway. If we get reports that the representation string is annoying, though, I'll open another ticket.

Otherwise ENTHUSIASTIC positive review for a great addition. Very minor changes (including putting a helper function somewhere it won't hurt doctest coverage, and a few typesetting things) attached in a reviewer patch. Documentation now looks great, passes tests.

[kcrisman]

Attachment: trac_9076-arc-reviewer.patch.gz

Apply after initial patch

[kcrisman]

comment:15

To release manager - this should apply fine to 4.5.2. Apply arc patch first, then arc-reviewer patch.

[e14f4152-4982-4ace-8c95-73a0599b109b]

comment:16

Please update attachment: trac_9076-arc.patch with a more descriptive commit string.

[kcrisman]

comment:17

Replying to @qed777:

Please update attachment: trac_9076-arc.patch with a more descriptive commit string.

This should get in, so here is a hand-edited version. If this doesn't apply, we'll have to wait for the original author to do it, since I can never get an actual created patch to have someone else's information. Release manager can revert to positive review if this works.

[kcrisman]

Same as other one, but better commit message

[e14f4152-4982-4ace-8c95-73a0599b109b]

comment:18

Attachment: trac_9076-arc.2.patch.gz

[e14f4152-4982-4ace-8c95-73a0599b109b]

Merged: sage-4.6.alpha1