Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#19953 closed enhancement (fixed)

Add pictures to plot.py

Reported by: jhonrubia6 Owned by:
Priority: minor Milestone: sage-7.1
Component: documentation Keywords: plot, docstring
Cc: jmantysalo, egourgoulhon, kcrisman Merged in:
Authors: Javier Honrubia González Reviewers: Eric Gourgoulhon, John Palmieri
Report Upstream: N/A Work issues:
Branch: 3a999e9 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jhonrubia6)

Modify the documentation of plot.py to incorporate pictures of the examples which produce plot

Change History (28)

comment:1 Changed 7 years ago by jhonrubia6

Authors: Javier Honrubia Gonzalez
Component: PLEASE CHANGEdocumentation
Description: modified (diff)
Keywords: plot docstring added
Priority: majorminor
Type: PLEASE CHANGEenhancement

comment:2 Changed 7 years ago by jmantysalo

Cc: jmantysalo added

comment:3 Changed 7 years ago by egourgoulhon

Cc: egourgoulhon added

comment:4 Changed 7 years ago by kcrisman

Authors: Javier Honrubia Gonzalez
Cc: kcrisman added

comment:5 Changed 7 years ago by jhonrubia6

Since

class GraphicsArray(WithEqualityById, SageObject)

has not plot method, defined one to return self

    def plot(self):
        """
        Draw a 2D plot of this graphics object, which just returns this
        object since this is already a 2D graphics object.

        EXAMPLES::

            place some example here
            
        """
        return self

What do you suggest? Do I create another ticket for that or just leave it in this ticket?

comment:6 Changed 7 years ago by jmantysalo

There is at least #18471 for this thing; please mark it as duplicate when you are done.

comment:7 Changed 7 years ago by jhonrubia6

Branch: u/jhonrubia6/add_pictures_to_plot_py

comment:8 Changed 7 years ago by kcrisman

Authors: Javier Honrubia Gonzalez
Commit: 6ed2d32c6bb49cf93156bbc2ce10323a1decbabf

New commits:

6ed2d32Partial commit with 85 graphics

comment:9 Changed 7 years ago by jhonrubia6

I included some 87 graphics in this commit. I would appreciate your comments. It is beginning to be a slow process, rewarding for me, but I ignore if we want to pay the price when building the reference documentation. A compromise solution would be not to generate all the graphics which have the comment #long time (good hint!), in addition to those figures which cannot be built since the implied functions do not have a plot() function.

comment:10 Changed 7 years ago by jhonrubia6

kind of "Go" or "Not Go" decission

Last edited 7 years ago by jhonrubia6 (previous) (diff)

comment:11 Changed 7 years ago by kcrisman

I have only been paying a little attention to this, but I would say that it might be better to have an on/off switch so that many developers don't have to waste time waiting for this. But so that the online documentation has full pictures. ?

comment:12 in reply to:  11 Changed 7 years ago by jhonrubia6

Replying to kcrisman:

I have only been paying a little attention to this, but I would say that it might be better to have an on/off switch so that many developers don't have to waste time waiting for this. But so that the online documentation has full pictures. ?

There is an option

sage --docbuild --no-plot

documented as "do not include graphics auto-generated using the 'plot' markup". I just tested it and it works.

comment:13 Changed 7 years ago by kcrisman

That sounds familiar. So then the question is which one should be default? I think you have already linked to the discussion about that somewhere.

PS there is at least one typo:

-    sage: plot(x**2, (x,0,3), ticks=[[1,2.5],pi/2], tick_formatter=[["$x_1$","$x_2$"],pi]) # long time
+    sage: plot(x2, (x,0,3), ticks=[[1,2.5],pi/2], tick_formatter=[["$x_1$","$x_2$"],pi]) # long time

will definitely raise an error about x2.

comment:14 Changed 7 years ago by git

Commit: 6ed2d32c6bb49cf93156bbc2ce10323a1decbabfa35837d720ebb545cd2f62d10741a7a75c437772

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

a35837dAdded all the 'drawable' pictures in plot.py

comment:15 in reply to:  13 Changed 7 years ago by jhonrubia6

Description: modified (diff)
Status: newneeds_review

Replying to kcrisman:

That sounds familiar. So then the question is which one should be default? I think you have already linked to the discussion about that somewhere.

PS there is at least one typo:

-    sage: plot(x**2, (x,0,3), ticks=[[1,2.5],pi/2], tick_formatter=[["$x_1$","$x_2$"],pi]) # long time
+    sage: plot(x2, (x,0,3), ticks=[[1,2.5],pi/2], tick_formatter=[["$x_1$","$x_2$"],pi]) # long time

will definitely raise an error about x2.

typo corrected, thanx

Last edited 7 years ago by jhonrubia6 (previous) (diff)

comment:16 Changed 7 years ago by egourgoulhon

Reviewers: Eric Gourgoulhon
Status: needs_reviewneeds_work

This is very nice; thank you for this work!

I've noticed some doctest error though: with sage 7.1.beta3, ./sage -t --long src/sage/plot/plot.py results in Error: TAB character found at line 1518 Indeed there are 2 tab characters at that line.

Moreover, there are spurious blank spaces in that file. You should suppress them by telling your editor to remove trailing whitespaces.

comment:17 Changed 7 years ago by git

Commit: a35837d720ebb545cd2f62d10741a7a75c43777242b9e4ccb4cf7a5cc6c2f93f304359d34059e23d

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

42b9e4cRemoved trailing blanks and spurious tabs

comment:18 in reply to:  16 Changed 7 years ago by jhonrubia6

Replying to egourgoulhon: Thank you for your feedback

I've noticed some doctest error though: with sage 7.1.beta3, ./sage -t --long src/sage/plot/plot.py results in Error: TAB character found at line 1518 Indeed there are 2 tab characters at that line.

not any more :-)

Moreover, there are spurious blank spaces in that file. You should suppress them by telling your editor to remove trailing whitespaces.

also gone.

comment:19 Changed 7 years ago by jhonrubia6

Status: needs_workneeds_review

comment:20 Changed 7 years ago by egourgoulhon

Status: needs_reviewpositive_review

Thanks for the changes. Everything seems fine. I've also checked that the pdf doc produced with ./sage -docbuild reference/plotting pdf is correctly generated, with all the figures included.

Regarding the extra CPU time discussed in comment:9 to comment:15, it took only 1 min 30 s to generate all the html plot section on my computer, which seems acceptable, given the huge benefit of having nice figures in the doc. So I suggest to leave the picture generation as the default and I am setting a positive review. Thanks again for this work!

comment:21 Changed 7 years ago by jhpalmieri

Status: positive_reviewneeds_work

I object to the change in plot/graphics.py. Either add a real example or don't include an EXAMPLES block. There should also be a newline before the start of the method.

comment:22 Changed 7 years ago by git

Commit: 42b9e4ccb4cf7a5cc6c2f93f304359d34059e23d3a999e90a1fe6132e5ca0d2f903fe370ff5d8b14

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

3a999e9Example added to the plot method in graphicsarray

comment:23 in reply to:  21 Changed 7 years ago by jhonrubia6

Replying to jhpalmieri:

I object to the change in plot/graphics.py. Either add a real example or don't include an EXAMPLES block. There should also be a newline before the start of the method.

you are right. I forgot to add the example, thank you for pointing it out. I think it is fixed now with an example.

comment:24 Changed 7 years ago by jhonrubia6

Status: needs_workneeds_review

comment:25 Changed 7 years ago by jhpalmieri

Status: needs_reviewpositive_review

That looks good to me, thanks.

comment:26 Changed 7 years ago by jhonrubia6

Reviewers: Eric GourgoulhonEric Gourgoulhon,John Palmieri

comment:27 Changed 7 years ago by vbraun

Branch: u/jhonrubia6/add_pictures_to_plot_py3a999e90a1fe6132e5ca0d2f903fe370ff5d8b14
Resolution: fixed
Status: positive_reviewclosed

comment:28 Changed 7 years ago by jdemeyer

Authors: Javier Honrubia GonzalezJavier Honrubia González
Commit: 3a999e90a1fe6132e5ca0d2f903fe370ff5d8b14
Reviewers: Eric Gourgoulhon,John PalmieriEric Gourgoulhon, John Palmieri
Note: See TracTickets for help on using tickets.