plot ellipses

[videlec]

Adding a primitive for plot ellipses that wraps the existing patch of matplotlib.

This is approximately the same stuff as the patch #9076 for plotting arcs (of circle and ellipse).

CC: @kcrisman @jasongrout

Component: geometry

Keywords: plot, ellipse

Author: Vincent Delecroix

Reviewer: Karl-Dieter Crisman

Merged: sage-4.6.alpha1

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

[kcrisman]

comment:4

This looks nice overall too, but again some things needed for best results.

• Class def examples for reference guide

• 'circle' still shows up a few times

• This _repr_ method is better than the arc one!

• plot3d should open ticket or test NotImplementedError

• I like that options are given explicitly in arc(), as well as test of NotImplementedError

I'll try to work through the details of the get_min_max_data and test thoroughly on this and #9076 as soon as these things are addressed, because in general they're both good wraps and add much-needed functionality. Good work!

[kcrisman]

Reviewer: Karl-Dieter Crisman

[videlec]

comment:5

Thank you for this careful review

• Class def examples for reference guide

Done, if you mean examples in the docstring of the class Ellipse

• 'circle' still shows up a few times

No more (I hope)

• plot3d should open ticket or test NotImplementedError

I will. But as I really do not like the one it is implemented for circle for many different reasons I don't know how general should be the corresponding ticket...

• I like that options are given explicitly in arc(), as well as test of NotImplementedError

Now there is. And I add a link from the sage.plot.plot

I'll try to work through the details of the get_min_max_data and test thoroughly on this and #9076 as soon as these things are addressed, because in general they're both good wraps and add much-needed functionality.

The get_min_max_data for ellipse is just obtained by computing corresponding critical points. This is not the good way for arc but I will make an effort for it (as it is not too much complicate).

[videlec]

comment:6

It appears that the get_min_max data is False. I'm working on it (post in few minutes)...

[videlec]

comment:7

The bounding box seems to work now. I joined a worksheet that perform a lot of drawings.

[kcrisman]

comment:8

See #9076 for comments on bounding box and worksheet, though for this ticket I think it's ok. Obviously the fmod function can be used here too, as well as already using math.pi since it's imported (I think this should work) and initializing the radii etc. as just the input numbers, waiting to float them until _render... and so forth. Very nice work otherwise.

This patch also depends on #9076, for others who might test it.

[videlec]

Attachment: trac_9203-ellipse.patch.gz

[videlec]

worksheet that tests the bounding box of arcs and ellipses

[videlec]

comment:9

Attachment: arcs and ellipses.sws.gz

Replying to @kcrisman:

See #9076 for comments on bounding box and worksheet, though for this ticket I think it's ok. Obviously the fmod function can be used here too, as well as already using math.pi since it's imported (I think this should work) and initializing the radii etc. as just the input numbers, waiting to float them until _render... and so forth. Very nice work otherwise.

This patch also depends on #9076, for others who might test it.

[kcrisman]

comment:11

Positive review! This will be great.

To release manager: very minor reviewer patch to be applied after trac_9023-ellipse patch. This depends on #9076.

[kcrisman]

Attachment: trac_9203-ellipse-reviewer.patch.gz

Apply after initial patch

[kcrisman]

comment:12

Also, see ticket #9719 for a followup to the awesome worksheet.

[qed777]

comment:13

Please update attachment: trac_9203-ellipse.patch with a more descriptive commit string.

[kcrisman]

comment:14

Replying to @qed777:

Please update attachment: trac_9203-ellipse.patch with a more descriptive commit string.

The following patch is simply a hand-edited version to include a better commit message - it was not actually committed. If that doesn't work/apply, we'll have to wait for the author to do this - but it would be really great to get this in! Release manager can revert to positive review if this is satisfying.

[kcrisman]

With better commit message, otherwise same

[qed777]

comment:15

Attachment: trac_9203-ellipse.2.patch.gz

[qed777]

comment:16

Replying to @kcrisman:

Replying to @qed777:

Please update attachment: trac_9203-ellipse.patch with a more descriptive commit string.

The following patch is simply a hand-edited version to include a better commit message - it was not actually committed. If that doesn't work/apply, we'll have to wait for the author to do this - but it would be really great to get this in! Release manager can revert to positive review if this is satisfying.

Thanks for updating the patch.

Since the 4.5.3 series is now in feature freeze --- it's just open to blocker problems such as build errors, doctest fixes, etc. --- and we'll merge the PARI upgrade into 4.6.alpha0, it's very likely that merging this ticket and #9076 will have to wait until 4.6.alpha1, at least.

[qed777]

Merged: sage-4.6.alpha1