Add pictures to hyperbolic_geodesic.py

[sagetrac-jhonrubia6]

Using ..PLOT:: incorporate pictures to illustrate the different methods in the module

Component: documentation

Keywords: hyperbolic

Author: Javier Honrubia González

Branch/Commit: e532ed8

Reviewer: Travis Scrimshaw

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

[sagetrac-jhonrubia6]

comment:1

apparently, we have to rename show methods to plot for sphinx_plot() to work

[videlec]

comment:2

The semantic of plot and show are different:

• plot should return a graphic object
• show does actually shows the object (the way it is shown depends on the Sage mode used, i.e. console, notebook, etc)

You can have a look at graphs

sage: G = graphs.PetersenGraph()
sage: P = G.plot()   # a plot object
sage: type(P)
<class 'sage.plot.graphics.Graphics'>
sage: R = P.show()   # returns None
Launched png viewer for Graphics object consisting of 26 graphics primitives
sage: R is None
True

And note that a graphics object as P above as a plot method (that return itself) and a show method that actually shows the graphics.

[sagetrac-jhonrubia6]

comment:3

I think I understand but as it is now

sage: PD = HyperbolicPlane().PD()
sage: g = PD.get_geodesic(-0.5, 0.3+0.4*I).show()
sage: type(g)
<class 'sage.plot.graphics.Graphics'>

That is the reason why I propose the renaming

[videlec]

comment:4

This is indeed very wrong... please fix it!

[sagetrac-jhonrubia6]

comment:5

you suggested the renaming back in the original ticket #9439 comment 24 but somehow was lost in the subsequent work.
Ok. I will do that, and add the pictures

[sagetrac-jhonrubia6]

Branch: u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py

[sagetrac-jhonrubia6]

Commit: 9b25a82

[sagetrac-jhonrubia6]

Author: Javier Honrubia González

[sagetrac-jhonrubia6]

Changed keywords from none to hyperbolic

[sagetrac-jhonrubia6]

New commits:

9b25a82

Added pictures to the doc. Added some more examples illustrating different models and geodesics. Renamed erroneous show() methods by plot() methods

[tscrim]

comment:8

Here are my comments:

• You have to deprecate show if you are going to outright remove it.

• I'm -1 on using show(P) in the doctests; IMO it is better to return the plot as it actually gets doctested in a fashion.

• Why this change:

  -            sage: g = HyperbolicPlane().PD().random_geodesic()
  +            sage: PD = HyperbolicPlane().PD()
  +            sage: g = PD.get_geodesic(-0.3+0.4*I,+0.7-0.1*I)

  I think the random test is somewhat better. However, I think you should add an additional test with this specific geodesic and give the commands for the user to reproduce your picture.

• Can you explain this change:

  -            sage: h = PD.get_geodesic(4/5*I + 3/5, 9/13*I + 6/13)
  +            sage: h = PD.get_geodesic(4/5*I + 3/5, I)

• Replace :math:`x2+y2-z^2=-1` with `x^2 + y^2 - z^2 = -1`.

• I don't understand the point of the comments in the .. PLOT:: directives. Moreover, the numbering either has no meaning or it will loose meaning as soon as someone adds another plot.

[sagetrac-jhonrubia6]

comment:9

Replying to @tscrim:

Here are my comments:

• You have to deprecate show if you are going to outright remove it.

Ok

• I'm -1 on using show(P) in the doctests; IMO it is better to return the plot as it actually gets doctested in a fashion.

Ok, I also had my doubts.

• Why this change:

  -            sage: g = HyperbolicPlane().PD().random_geodesic()
  +            sage: PD = HyperbolicPlane().PD()
  +            sage: g = PD.get_geodesic(-0.3+0.4*I,+0.7-0.1*I)

  I think the random test is somewhat better. However, I think you should add an additional test with this specific geodesic and give the commands for the user to reproduce your picture.

Agreed. I changed the random_geodesic to make consistent and good-looking into the doc. I think you are right, we better move the random_geodesic to the test directive.

• Can you explain this change:

  -            sage: h = PD.get_geodesic(4/5*I + 3/5, 9/13*I + 6/13)
  +            sage: h = PD.get_geodesic(4/5*I + 3/5, I)

Well, better looking picture I guess.

• Replace :math:`x2+y2-z^2=-1` with `x^2 + y^2 - z^2 = -1`.

Ok

• I don't understand the point of the comments in the .. PLOT:: directives. Moreover, the numbering either has no meaning or it will loose meaning as soon as someone adds another plot.

oops, it was debugging code, I'll remove it.

[sagetrac-git]

Changed commit from 9b25a82 to 562736f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

562736f

Modifications as per reviewer comments

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:12

One little thing: TEST: -> TESTS::. Once that is done, you can set a positive review on my behalf.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

e532ed8

Modified TEST block

[sagetrac-git]

Changed commit from 562736f to e532ed8

[vbraun]

Changed branch from u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py to e532ed8