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: sage-7.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:

Status badges

Description

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

Change History (15)

comment:1 Changed 5 years ago by jhonrubia6

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

comment:2 Changed 5 years ago by vdelecroix

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.

comment:3 Changed 5 years ago by jhonrubia6

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 vdelecroix

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

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:5 Changed 5 years ago by jhonrubia6

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 jhonrubia6

  • Branch set to u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py

comment:7 Changed 5 years ago by jhonrubia6

  • Authors set to Javier Honrubia González
  • Commit set to 9b25a8217de93b8dda178e4767cc709bb0dbf1e4
  • Keywords documentation hyperbolic geometry added
  • Milestone changed from sage-7.2 to sage-7.3
  • Status changed from new to needs_review

New commits:

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

comment:8 follow-up: Changed 5 years ago by tscrim

  • 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:
    -            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:`x^2+y^2-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.

comment:9 in reply to: ↑ 8 Changed 5 years ago by jhonrubia6

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:`x^2+y^2-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.

comment:10 Changed 5 years ago by git

  • Commit changed from 9b25a8217de93b8dda178e4767cc709bb0dbf1e4 to 562736fecd110cc43a42ba02d93c8ce09138200d

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

562736fModifications as per reviewer comments

comment:11 Changed 5 years ago by jhonrubia6

  • Status changed from needs_work to needs_review

comment:12 Changed 5 years ago by tscrim

  • 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 git

  • Commit changed from 562736fecd110cc43a42ba02d93c8ce09138200d to e532ed8230d815a1ae52ee58ca8ea011a5d704e8

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

e532ed8Modified TEST block

comment:14 Changed 5 years ago by jhonrubia6

  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/jhonrubia6/add_pictures_to_hyperbolic_geodesic_py to e532ed8230d815a1ae52ee58ca8ea011a5d704e8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.