Opened 5 years ago
Closed 5 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 5 years ago by
comment:2 Changed 5 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 5 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:4 Changed 5 years ago by
This is indeed very wrong... please fix it!
comment:5 Changed 5 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 5 years ago by
 Branch set to u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py
comment:7 Changed 5 years ago by
 Commit set to 9b25a8217de93b8dda178e4767cc709bb0dbf1e4
 Keywords documentation hyperbolic geometry added
 Milestone changed from sage7.2 to sage7.3
 Status changed from new to 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 5 years ago by
 Status changed from needs_review to 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 in reply to: ↑ 8 Changed 5 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 5 years ago by
 Commit changed from 9b25a8217de93b8dda178e4767cc709bb0dbf1e4 to 562736fecd110cc43a42ba02d93c8ce09138200d
Branch pushed to git repo; I updated commit sha1. New commits:
562736f  Modifications as per reviewer comments

comment:11 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
One little thing: TEST:
> TESTS::
. Once that is done, you can set a positive review on my behalf.
comment:13 Changed 5 years ago by
 Commit changed from 562736fecd110cc43a42ba02d93c8ce09138200d to e532ed8230d815a1ae52ee58ca8ea011a5d704e8
Branch pushed to git repo; I updated commit sha1. New commits:
e532ed8  Modified TEST block

comment:14 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:15 Changed 5 years ago by
 Branch changed from u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py to e532ed8230d815a1ae52ee58ca8ea011a5d704e8
 Resolution set to fixed
 Status changed from positive_review to closed
apparently, we have to rename
show
methods toplot
forsphinx_plot()
to work