Opened 7 years ago
Closed 7 years ago
#20530 closed enhancement (fixed)
Add pictures to hyperbolic_geodesic.py
Reported by:  jhonrubia6  Owned by:  

Priority:  minor  Milestone:  sage7.3 
Component:  documentation  Keywords:  documentation, hyperbolic, geometry 
Cc:  Merged in:  
Authors:  Javier Honrubia González  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e532ed8 (Commits, GitHub, GitLab)  Commit:  e532ed8230d815a1ae52ee58ca8ea011a5d704e8 
Dependencies:  Stopgaps: 
Description
Using ..PLOT:: incorporate pictures to illustrate the different methods in the module
Change History (15)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
The semantic of plot
and show
are different:
plot
should return a graphic objectshow
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.
comment:3 Changed 7 years ago by
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
comment:5 Changed 7 years ago by
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
comment:6 Changed 7 years ago by
Branch:  → u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py 

comment:7 Changed 7 years ago by
Authors:  → Javier Honrubia González 

Commit:  → 9b25a8217de93b8dda178e4767cc709bb0dbf1e4 
Keywords:  documentation hyperbolic geometry added 
Milestone:  sage7.2 → sage7.3 
Status:  new → needs_review 
New commits:
9b25a82  Added pictures to the doc. Added some more examples illustrating different models and geodesics. Renamed erroneous show() methods by plot() methods

comment:8 followup: 9 Changed 7 years ago by
Status:  needs_review → needs_work 

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:
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.
 sage: g = HyperbolicPlane().PD().random_geodesic() + sage: PD = HyperbolicPlane().PD() + sage: g = PD.get_geodesic(0.3+0.4*I,+0.70.1*I)
 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:`x^2+y^2z^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.
comment:9 Changed 7 years ago by
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:
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. sage: g = HyperbolicPlane().PD().random_geodesic() + sage: PD = HyperbolicPlane().PD() + sage: g = PD.get_geodesic(0.3+0.4*I,+0.70.1*I)
Agreed. I changed the random_geodesic to make consistent and goodlooking 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:`x^2+y^2z^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.
comment:10 Changed 7 years ago by
Commit:  9b25a8217de93b8dda178e4767cc709bb0dbf1e4 → 562736fecd110cc43a42ba02d93c8ce09138200d 

Branch pushed to git repo; I updated commit sha1. New commits:
562736f  Modifications as per reviewer comments

comment:11 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:12 Changed 7 years ago by
Reviewers:  → Travis Scrimshaw 

One little thing: TEST:
> TESTS::
. Once that is done, you can set a positive review on my behalf.
comment:13 Changed 7 years ago by
Commit:  562736fecd110cc43a42ba02d93c8ce09138200d → e532ed8230d815a1ae52ee58ca8ea011a5d704e8 

Branch pushed to git repo; I updated commit sha1. New commits:
e532ed8  Modified TEST block

comment:14 Changed 7 years ago by
Status:  needs_review → positive_review 

comment:15 Changed 7 years ago by
Branch:  u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py → e532ed8230d815a1ae52ee58ca8ea011a5d704e8 

Resolution:  → fixed 
Status:  positive_review → closed 
apparently, we have to rename
show
methods toplot
forsphinx_plot()
to work