Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#17498 closed enhancement (fixed)

Pictures in the doc through ".. plot::" directive

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.5
Component: documentation Keywords:
Cc: vbraun, vdelecroix, tmonteil, jmantysalo, ​fredrik.johansson Merged in:
Authors: Nathann Cohen, John Palmieri, Thierry Monteil Reviewers: Nathann Cohen, John Palmieri, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: 0998e30 (Commits) Commit:
Dependencies: #17508 Stopgaps:

Description (last modified by ncohen)

With this branch, one can use the ".. PLOT::" directive defined in matplotlib through Sphinx.

As it expects the picture to be defined as a matplotlib one, I added a small function called 'draw' that translates a Sage object into a matplotlib one through a temporary .png file (I know, I know...)

While it works as it is, it is probably not implemented as it should.

Soooo let us fix this and use that feature !

Nathann

Associated sage-devel thread at: https://groups.google.com/d/topic/sage-devel/JnuAAd7iH2g/discussion

Change History (74)

comment:1 Changed 5 years ago by ncohen

  • Branch set to public/17498
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to b5848d11328b1b785f54789ba9ccd7bc6d4cc981

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

b5848d1trac #17498: Pictures in the doc through ".. plot::" directive

comment:3 Changed 5 years ago by ncohen

  • Description modified (diff)

comment:4 follow-ups: Changed 5 years ago by jdemeyer

This should be automatic (show the output of Sage plot commands in doctests), not manual.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by ncohen

This should be automatic (show the output of Sage plot commands in doctests), not manual.

True, but unless you know how to make it this is still better than what we have.

Version 0, edited 5 years ago by ncohen (next)

comment:6 in reply to: ↑ 5 Changed 5 years ago by jdemeyer

Replying to ncohen:

True, though I do not know how to make it work. Can you ?

No, I don't know anything about Sphinx. But perhaps somebody knows...

comment:7 follow-ups: Changed 5 years ago by jmantysalo

Would be nice to have an easy way to put several images to an array. For example to have posets P and Q so that P.is_foo() is true and Q.is_foo() is false, or to show ordinal products P*Q and Q*P.

comment:8 in reply to: ↑ 7 Changed 5 years ago by ncohen

Would be nice to have an easy way to put several images to an array.

Also True, but this should not be sphinx-related code: with this branch you can have in the doc "any picture that Sage can produce". Thus if you can produce in Sage a picture which is a combination of several pictures you can have it in the manual.

What you need is purely Sage code. This thing is only an interface between Sage and the .. PLOT:: directive defined in matplotlib.

Nathann

comment:9 in reply to: ↑ 7 Changed 5 years ago by jdemeyer

Replying to jmantysalo:

Would be nice to have an easy way to put several images to an array.

GraphicsArray does that.

comment:10 in reply to: ↑ 4 Changed 5 years ago by vdelecroix

Replying to jdemeyer:

This should be automatic (show the output of Sage plot commands in doctests), not manual.

I guess these are two different things. In a thematic tutorial for examples, you might want to have some pictures without showing the actual commands that provide them at first.

comment:11 Changed 5 years ago by schilly

At LMFDB, we convert Sage Plots P to an encoded string here. Maybe it produces better results this way? We also use StringIO instead of a temp file ... not sure if that makes sense here (in fact, I don't understand how the actual image ends up in the HTML anyways.)

comment:12 Changed 5 years ago by git

  • Commit changed from b5848d11328b1b785f54789ba9ccd7bc6d4cc981 to 8bbec182e0e68f4679dab359717fe92eb0388c5f

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

8bbec18trac #17498: No import, less margins

comment:13 Changed 5 years ago by ncohen

  • Description modified (diff)

comment:14 Changed 5 years ago by ncohen

At LMFDB, we convert Sage Plots P to an encoded string. Maybe it produces better results this way?

What do you mean ? You think that storing the picture in a file damages it ? :-P

(in fact, I don't understand how the actual image ends up in the HTML anyways.)

I would say that "it's all global variables anyway". When you want to plot a picture it seems that you have to import matplotlib.pyplot as plt and then you do things like plt.margins() or plt.axis('off'). So matplotlib probably takes that very object and makes it output its result in a file instead...

Anyway. Who could review this patch ? And we could start adding drawings to Sage's doc.

Nathann

comment:15 Changed 5 years ago by jmantysalo

  • Status changed from needs_review to needs_work

Needs few lines of documentation to http://www.sagemath.org/doc/developer/coding_basics.html#section-docstrings

I guess that mostly a placeholder for now. Later we need documentation about when and what kind of pictures to add, not just how to do it.

comment:16 Changed 5 years ago by vinceknight

/sub (Just listening in.)

comment:17 Changed 5 years ago by git

  • Commit changed from 8bbec182e0e68f4679dab359717fe92eb0388c5f to d02a544c4f3f01cee055fd445a75509806dfce2b

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

d02a544trac #17498: note in the developper's manual

comment:18 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:19 Changed 5 years ago by jmantysalo

Commit d02a544 is OK to me.

(Someone knowing graphics should do the real review to code.)

comment:20 Changed 5 years ago by git

  • Commit changed from d02a544c4f3f01cee055fd445a75509806dfce2b to bfdc22287e8f702e394753f0dbd55896d045282c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bfdc222trac #17498: note in the developper's manual

comment:21 follow-up: Changed 5 years ago by kcrisman

Yo! Do you think that using the built-in .matplotlib() method for all our graphics (well, 2d) would work? (For instance, we already usually have tight layout.) I would say that your draw function is more of a proof-of-concept, not really something we would want to actually add. But we could add instructions about how to use the PLOT:: directive right now to have them actually show up in the docs. One could create a major patch bomb by changing the sage/plot/ files to have all plots use this :)

Indeed, we shouldn't have to have the PLOT:: directive at all, but maybe that's too much to ask. Volker, you know the most about the display hook - any ideas for how to get that to show up automatically? I know there is a doctest mode, maybe there can be a sphinx mode as well... ?

comment:22 in reply to: ↑ 21 Changed 5 years ago by ncohen

Hello !

Yo! Do you think that using the built-in .matplotlib() method for all our graphics (well, 2d) would work? (For instance, we already usually have tight layout.)

I do not understand your question. Here is how the draw function currently works:

1) Take a Sage graphical object and save it as a png file 2) Load the png with matplotlib

Then, matplotlib's 'plot' directive knows how to handle it and add the pictures in the html documentation. Pretty simple from out point of view, and will work with anything we can turn into a jpg file.

In particular (though I do not know if this is what you asked) the 'tight layout' parameter will have an impact on the .jpg file we *produce*. The 'tight layout' I added in the code is a parameter of the picture produced by matplotlib from our jpg file.

I would say that your draw function is more of a proof-of-concept, not really something we would want to actually add.

Well. I would also prefer calls to .show() to produce plots in the doc, but really I do not know how to write this branch better than how it is done. If it turns out that nobody can rewrite it better, I would still prefer to have this 'draw' function around and be able to add pictures to the doc than nothing at all.

But we could add instructions about how to use the PLOT:: directive right now to have them actually show up in the docs.

Again I do not understand: do you mean instructions like the ones I added in my last commit ?

One could create a major patch bomb by changing the sage/plot/ files to have all plots use this :)

Still do not understand what you mean by "to have all plots use this". Note that even if we were able to turn all calls to .show() into a picture in the reference manual it would not necessarily be a good idea. I remember a time when peopl complained a bit about the size of our documentation, and.... Adding pictures to that would probably mean more than a couple of megabytes.

Plus it would mean having Sphinx go through all doctests when building the doc, and that's not a good idea either.

Good evening,

Nathann

comment:23 follow-up: Changed 5 years ago by jhpalmieri

The "doctest" syntax for matplotlib's plot directive sounds interesting:

  3. Using **doctest** syntax::

       .. plot::
          A plotting example:
          >>> import matplotlib.pyplot as plt
          >>> plt.plot([1,2,3], [4,5,6])

but I can't figure out how to get it to work, mainly because I don't know how to convert a Sage graphics object into something matplotlib can plot. For example,

sage: C = grsphs.PetersenGraph().plot()
sage: fig = C.matplotlib() # looks good, right?
sage: fig.show()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
...
AttributeError: 'NoneType' object has no attribute 'manager'
Figure.show works only for figures managed by pyplot, normally created by pyplot.figure().

Understanding how to do this would let us use just a few lines like

.. plot::

    >>> from sage.all_cmdline import *
    >>> graphs.CubeGraph(8).plot().matplotlib().show()

instead of having to manually save to a temporary file.

comment:24 Changed 5 years ago by kcrisman

Sorry for perhaps being opaque. Here is the usual wish from people:

Why don't the plots that appear when one evaluates the live documentation appear already in the static documentation?

Your solution works great (I haven't tried it, but I trust you!) for adding in a few plots at crucial points in the doc. But in files that have a lot of plots, it would make it very wearying to have that directive in there everywhere, and as you point out size might be a factor as well (though that hasn't stopped us before).

I didn't think about Sphinx needing to go through doctests to build doc. That is a good point.

What about this. Could we use a different name for the command, maybe? Something that like include_plot_for_doc? (And hopefully just plot.plot().matplotlib(), though one would want to test this with things that are already Graphics objects as well.)

Edit: I see John's question - savefig may work if we have the right backend, I think.

comment:26 in reply to: ↑ 23 Changed 5 years ago by schilly

Replying to jhpalmieri:

but I can't figure out how to get it to work, mainly because I don't know how to convert a Sage graphics object into something matplotlib can plot.

That's exactly what I've posted earlier about LMFDB here.

from matplotlib.backends.backend_agg import FigureCanvasAgg
fig = P.matplotlib()
fig.set_canvas(FigureCanvasAgg(fig))
fig.savefig(..., format='png')

LMFDB also uses StringIO to avoid the creation of temporary files. I think that's a good thing in general and only has benefits. I just don't know how the underlying mechanism here works, but I would recommend doing that.

comment:27 Changed 5 years ago by jhpalmieri

I think that savefig will work, but doesn't that save to a file? (Unless we use Harald's suggestion, that is.) The lines

  3. Using **doctest** syntax::

       .. plot::
          A plotting example:
          >>> import matplotlib.pyplot as plt
          >>> plt.plot([1,2,3], [4,5,6])

from matplotlib.sphinxext.plot_directive suggest that if we give it the right plotting command, whatever that is, we should be able to produce an image on the fly.

For me, it boils down to the fact that I don't understand Sage's graphics, I don't understand matplotlib's graphics, and I absolutely don't understand the interaction between the two.

Harald's suggestion seems good, though. I'd rather not create temporary files.

Another option: we could read all of the doctests once and create all of the missing pictures, and then ship the graphics files as part of the documentation source (as we do already with a handful of files). I'm imagining a new target in the Makefile which would somehow do this. Which is better: producing the pictures once and distributing them with every copy of Sage, or making the source distribution smaller and producing the pictures every time the documentation is built? Maybe I'm really asking about the long term plans: if we are only planning to add a few pictures, it doesn't matter too much (to me, anyway) how it's done. But if we are planning to add a lot of pictures, then we want to consider how best to automate it, and also what the costs to the developers and users will be: will docbuilding suddenly take much longer because we're building 500 plots each time, or will the source code be larger because we're including 500 plots? If we include the plots, what's the best format? '.sobj' and '.pdf' look like the smallest in a few examples I just tried...

comment:28 Changed 5 years ago by kcrisman

I think that we definitely don't want to include all possible graphics from doctests in a local Sage download (or build), some are quite large. And most people probably would only be using their local doc in the sagenb anyway, which has the live doc. (Does SMC have an analogous concept? I hope so.) However, in the mirrored documentation I think we definitely (if possible) do want it, so an optional target seems plausible.

What I'd like to see is to use Harald's idea here (would one need the correct backend, e.g. SAGE_MATPLOTLIB_GUI, built?) to test that it works at all.

As to formats, we would want something that works best on webpages. .sobj is not appropriate for that :) and I don't know how embedded pdfs look. svg might be too big. Currently when we do add them, we use png, but those are of course not scalable.

comment:29 Changed 5 years ago by ncohen

Helloooooo !

Just to say that I tried several times to user Harald's trick to not have to go through a temporary file, but that it made the margins larger again and that I did not achieve to avoid that :-/

Result at public/17498_nofile if anybody wants to try things.

Nathann

comment:30 Changed 5 years ago by vbraun

This is not about adding all plots from doctests, only from the special ..plot: sphinx environment.

Pictures should never be added to the repo if it can be avoided.

How about rename draw(plot) to sphinx_draw(plot) to make it clear that this is specific to the docbuilder. We could add an option to specify the appropriate output format, but anything that is not just png will need special care. For example, vector graphics output must be genenrated as svg + pdf for the html and pdf documentation.

For the doctest syntax to make sense we would have to support the sage: prompt instead of >>> and so on. This can be move to a separate ticket.

comment:31 Changed 5 years ago by ncohen

Hello !

How about rename draw(plot) to sphinx_draw(plot) to make it clear that this is specific to the docbuilder.

You will have this as soon as I find an internet connection without proxy. Can't use git at the moment. Something like in a couple of hours.

Nathann

comment:32 Changed 5 years ago by ncohen

Okay, turns out that even at home I now need a proxy to connect to internet. Even worse, Sage and Gmail seem to be the only pages I can load.

You can change the draw into sphinx_draw if you prefer, no problem from me. Turns out that I can't work on any branch at the moment -_-

Nathann

comment:33 follow-up: Changed 5 years ago by kcrisman

At least you can load Sage :-)

I'm fine with this ticket being just about supporting the addition of a few dynamically generated pictures, as long as the syntax is very clear that is what is going on. I would probably use it myself!

I like my suggestion of plot_for_doc a little better, but Volker's is fine too (I just don't see how this is 'draw'ing, but maybe I mean something different by that because I think of line drawings) - the point is to make it really clear this isn't a standard Sage function someone might erroneously try to use to make a picture in general (outside of doc).

comment:34 Changed 5 years ago by git

  • Commit changed from bfdc22287e8f702e394753f0dbd55896d045282c to 42a0b5a8a3af4789388810c0f92ecd13aceb80dc

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

42a0b5atrac #17498: Change the function's name, add a couple of examples

comment:35 in reply to: ↑ 33 Changed 5 years ago by ncohen

Yooooooo !

I like my suggestion of plot_for_doc a little better, but Volker's is fine too (I just don't see how this is 'draw'ing, but maybe I mean something different by that because I think of line drawings) - the point is to make it really clear this isn't a standard Sage function someone might erroneously try to use to make a picture in general (outside of doc).

Well, it is now 'sphinx_plot'.... And I also added a couple of examples in the graph section :-)

Nathann

comment:36 follow-up: Changed 5 years ago by kcrisman

How much time does this add to the doc build? Say, in the developer manual. Which reminds me, update developer manual, please :-)

Again, I am wondering whether this works with other plots, like plot(sin(x),(x,0,pi)) or something. Yes, I am too tired to try it now. But whatever is implemented should be robust in that sense, even if it's our fault for having several different ways to plot things (plotting with plot() versus .plot() methods).

I wonder also about importing mpl every time. Maybe once it's already imported it's not a big deal, but if there was one such import per file in the reference manual that could add quite a bit of time (I realize that assumes people have put such things in). You're sure you can't change the margins or layout before doing the savefig in the StringIO version? Too bad.

comment:37 in reply to: ↑ 36 Changed 5 years ago by ncohen

Hello !

How much time does this add to the doc build?

Well, nothing I guess if nobody uses it. Then it will depend on how many pictures we add.

Which reminds me, update developer manual, please :-)

Do you mean that I should update this ticket with #17508 one way or another ? Review one, I will update the other :-P

Again, I am wondering whether this works with other plots, like plot(sin(x),(x,0,pi)) or something. Yes, I am too tired to try it now.

Load the 'sphinx_plot' function in Sage. If it does not raise an exception on the object you give it then it works. All that is necessary is to turn this thing into a .png, that's all.

But whatever is implemented should be robust in that sense, even if it's our fault for having several different ways to plot things (plotting with plot() versus .plot() methods).

I don't totally grasp the dichotomy you have in mind :-P

I wonder also about importing mpl every time. Maybe once it's already imported it's not a big deal, but if there was one such import per file in the reference manual that could add quite a bit of time (I realize that assumes people have put such things in). You're sure you can't change the margins or layout before doing the savefig in the StringIO version? Too bad.

Well, I was not able to.. It must be possible I expect, but well O_o

Nathann

comment:38 Changed 5 years ago by git

  • Commit changed from 42a0b5a8a3af4789388810c0f92ecd13aceb80dc to 361f9a8a79aa51fa1f0d39dc08be0a379fb9b30a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5f4d0b3trac #17508: Reformat the 'docstring' section of the developer's manual
d8ba887trac #17508: Reviewer's remarks
8728734trac #17508: Reviewer's remarks 2
361f9a8trac #17498: Pictures in the doc through ".. plot::" directive

comment:39 Changed 5 years ago by ncohen

  • Dependencies set to #17508

Rebased !

comment:40 follow-up: Changed 5 years ago by ncohen

YOooooo ! Is anything else needed ?

Nathann

comment:41 Changed 5 years ago by kcrisman

I am happy with the concept, but won't be looking at the actual plot creation code etc. right now. If someone else wants to look at that but doesn't care about the concept, then that should be sufficient for positive review.

comment:42 in reply to: ↑ 40 Changed 5 years ago by tmonteil

  • Status changed from needs_review to needs_work

Replying to ncohen:

YOooooo ! Is anything else needed ?

Concerning requirements (i did not test the branch yet, only read its content):

  • This was already discussed in comments 27 and 28: there should be a way to turn this feature on/off from the sage -docbuild and the make doc commands, since some binaries need to stay small. For example, one could add a htmlpng (or whatever htmlplot, through at some point one could insert e.g. svg images) docbuild format (and also trigger the plotting for the existing web format), which leads to calls like:
sage -docbuild reference htmlpng
  • The new sphinx_plot function is not documented nor tested.

Concerning the discussion about the syntax, a possibility which could make syntax more uniform (hence easier to remember) could be to have a similar syntax as in passing options to the doctests, e.g.

::
    graphs.PetersenGraph().plot()     # plot

Actually, both syntax could coexist, because there are two use cases with two different behaviors:

  • a ..PLOT:: directive to insert plots within some classical text, so that the code that builds the image does not appear, as for the currently used ..image:: directive (see e.g. in src/sage/homology/simplicial_complex.py and the resulting page).
  • a # plot comment within the doctests, so that plots appear as the result of the doctest (resp. under the corresponding cell in the live doc), while letting the code visible (resp. still executable in the live cells).

Probably not for this ticket, we should take the opportunity to remove hardcoded png images from the git tree that are used by the ..image:: directive, and build them instead (at least for the plots that were obtained with Sage). However, the old ..image:: hardcoded way had the advantage that images do not depend on bugs, so there should be some way to kind of test the automatically produced pictures (from their size?, from similarities with previous build?), or at least to provide a way to see them all at once to have a visual check before releasing and pushing to the official doc.

comment:43 Changed 5 years ago by ncohen

Yo !

  • This was already discussed in comments 27 and 28: there should be a way to turn this feature on/off from the sage -docbuild and the make doc commands, since some binaries need to stay small.

I do not know how to do that. At first I thought that it would be impossible without rewriting matplotlib's plot directive, but then it seems that it accepts a 'nofigs' option to do just that. I have no idea how to change its value, however (we cannot touch the file directly, it is not Sage code)

  • The new sphinx_plot function is not documented nor tested.

Well, it is not even a function but a string. And I do not see what you want me to test, the function returns no output.

(sage -coverage does not report the function)

Concerning the discussion about the syntax, a possibility which could make syntax more uniform (hence easier to remember) could be to have a similar syntax as in passing options to the doctests, e.g.

It would be nice, but I have no idea how to do that you want of me. Do you ?

I also expect that we cannot ask Sphinx to do that unless the doctests are in some kind of environment (like ".. PLOT")

Nathann

comment:44 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:45 Changed 5 years ago by jhpalmieri

Something like this might do it:

diff --git a/Makefile b/Makefile
index 254c36c..7154e75 100644
--- a/Makefile
+++ b/Makefile
@@ -59,6 +59,9 @@ doc: doc-html
 doc-html: build
        $(PIPE) "./sage --docbuild --no-pdf-links all html $(SAGE_DOCBUILD_OPTS) 2>&1" "tee -a logs/dochtml.log"
 
+doc-html-no-pix: build
+       $(PIPE) "./sage --docbuild --no-pdf-links all html-no-pix $(SAGE_DOCBUILD_OPTS) 2>&1" "tee -a logs/dochtml.log"
+
 doc-html-mathjax: build
        $(PIPE) "./sage --docbuild --no-pdf-links all html -j $(SAGE_DOCBUILD_OPTS) 2>&1" "tee -a logs/dochtml.log"
 
diff --git a/src/doc/common/builder.py b/src/doc/common/builder.py
index c817d86..36e45f3 100644
--- a/src/doc/common/builder.py
+++ b/src/doc/common/builder.py
@@ -1613,6 +1613,12 @@ if __name__ == '__main__':
     # Set up Intersphinx cache
     C = IntersphinxCache()
 
+    if type == 'html-no-pix':
+        type = 'html'
+        os.environ['SPHINX-INCLUDE-PLOTS'] = 'False'
+    else:
+        os.environ['SPHINX-INCLUDE-PLOTS'] = 'True'
+
     # Get the builder and build.
     try:
         getattr(get_builder(name), type)()
diff --git a/src/doc/common/conf.py b/src/doc/common/conf.py
index 4bf8a1d..11be416 100644
--- a/src/doc/common/conf.py
+++ b/src/doc/common/conf.py
@@ -23,9 +23,28 @@ extensions = ['inventory_builder', 'multidocs',
               'sphinx.ext.inheritance_diagram', 'sphinx.ext.todo',
               'sphinx.ext.extlinks', 'matplotlib.sphinxext.plot_directive']
 
-from matplotlib.sphinxext import plot_directive
-from sage.plot.misc import plot_pre_code
+# This code is executed before each ".. PLOT::" directive in the Sphinx
+# documentation. It defines a 'draw' functions that displays a Sage object
+# through mathplotlib, so that it will be displayed in the HTML doc
+plot_pre_code = """
+def sphinx_plot(plot):
+    import matplotlib.image as mpimg
+    from sage.misc.temporary_file import tmp_filename
+    import matplotlib.pyplot as plt
+    if os.environ.get('SPHINX-INCLUDE-PLOTS', False) == 'True':
+        fn = tmp_filename(ext=".png")
+        plot.plot().save(fn)
+        img = mpimg.imread(fn)
+        plt.imshow(img)
+        plt.margins(0)
+        plt.axis("off")
+        plt.tight_layout(pad=0)
+
+from sage.all_cmdline import *
+"""
+
 plot_html_show_formats = False
 
 # We do *not* fully initialize intersphinx since we call it by hand
diff --git a/src/sage/plot/misc.py b/src/sage/plot/misc.py
index e6e2058..79bfc30 100644
--- a/src/sage/plot/misc.py
+++ b/src/sage/plot/misc.py
@@ -377,22 +377,3 @@ def get_matplotlib_linestyle(linestyle, return_type):
                              "'dashed', 'dotted', dashdot', 'None'}, "
                              "respectively {'-', '--', ':', '-.', ''}"%
                              (linestyle))
-
-# This code is executed before each ".. PLOT::" directive in the Sphinx
-# documentation. It defines a 'sphinx_plot' function that displays a Sage object
-# through mathplotlib, so that it will be displayed in the HTML doc.
-plot_pre_code = """
-def sphinx_plot(plot):
-    import matplotlib.image as mpimg
-    from sage.misc.temporary_file import tmp_filename
-    import matplotlib.pyplot as plt
-    fn = tmp_filename(ext=".png")
-    plot.plot().save(fn)
-    img = mpimg.imread(fn)
-    plt.imshow(img)
-    plt.margins(0)
-    plt.axis("off")
-    plt.tight_layout(pad=0)
-
-from sage.all_cmdline import *
-"""

Unfortunately, the presence or lack of pictures is cached in src/doc/output/doctrees, so to see any difference between make doc-html and make doc-html-no-pix, it's probably safest to first run make doc-clean.

By the way, why not put "sphinx_plot" directly in conf.py, rather than import it from sage.plot.misc? Also, the comment before it should be changed to mention "sphinx_plot" instead of "draw".

comment:46 Changed 5 years ago by jhpalmieri

By "this might do it", I mean that this would define a doc-html-no-pix target in the top-level Makefile which would skip all of these plots. I think that it would still include the (as far as I can tell, always broken, even with Nathann's version) "Source code" links. I don't know how to turn those off.

comment:47 Changed 5 years ago by ncohen

Helloooooooo !

Something like this might do it:

Could you add this as a commit or modify the branch ?

Thanks,

Nathann

comment:48 Changed 5 years ago by ncohen

P.S.: I need to use an internet proxy which prevents me from using git, and disconnected me every second from trac. In order to post a comment I must click several times on 'login' until it works, scroll fast to the bottom of the page, paste my comment and hope when I clicked on 'submit' that I have not been logged out in the meantime. It takes something like 5-6 attempts each time I want to add something T_T

comment:49 Changed 5 years ago by git

  • Commit changed from 361f9a8a79aa51fa1f0d39dc08be0a379fb9b30a to e3a86aace7ee116577698d6e2146a7eeec31c17f

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

ba5b417Merge branch 'develop' into t/17498/public/17498
e3a86aa#17498: add doc-html-no-pix Makefile target

comment:50 Changed 5 years ago by git

  • Commit changed from e3a86aace7ee116577698d6e2146a7eeec31c17f to d7ae73cdec62e1d461fb951ef962746ddbdfd1b7

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

d7ae73c#17498: typo

comment:51 follow-up: Changed 5 years ago by jhpalmieri

Okay, I've modified the branch.

comment:52 in reply to: ↑ 51 Changed 5 years ago by ncohen

Okay, I've modified the branch.

Thank you !

comment:53 Changed 5 years ago by ncohen

Guys ?... Anybody for a review ? :-P

comment:54 Changed 4 years ago by ncohen

Guys ?...

comment:55 follow-up: Changed 4 years ago by jhpalmieri

I still think that there should be a better way to do this, a way that doesn't involve creating a temporary file, but since we can't figure it out, I think we can merge this. So a positive review for all parts I didn't write myself.

comment:56 in reply to: ↑ 55 Changed 4 years ago by ncohen

  • Authors changed from Nathann Cohen to Nathann Cohen, John Palmieri
  • Reviewers set to Nathann Cohen, John Palmieri
  • Status changed from needs_review to positive_review

Yooooooooo !

I still think that there should be a better way to do this, a way that doesn't involve creating a temporary file, but since we can't figure it out, I think we can merge this. So a positive review for all parts I didn't write myself.

I also think that we will probably rewrite this a couple of times in the future. This being said, with this included we get a new feature: we can add pictures to the doc easily, and this will be cool very quickly.

I tested your flag, recompiled all of Sage's doc each time, and it all seems good... Thanks !

Nathann

comment:57 follow-ups: Changed 4 years ago by tmonteil

  • Status changed from positive_review to needs_work

Hi,

sorry for not being reactive, i am quite seek currently so the time i can spend on Sage is lowered.

Replying to ncohen:

I do not know how to do that. At first I thought that it would be impossible without rewriting matplotlib's plot directive, but then it seems that it accepts a 'nofigs' option to do just that. I have no idea how to change its value, however (we cannot touch the file directly, it is not Sage code)

Not knowing how to fix this issue is not a sufficient condition to let the size of every Sage binary explode (nor putting the ticket back to needs_review as if no solution implies no problem). I was not claiming that you should fix it yourself, i was claiming that the ticket needs work, working on a ticket is a collaborative task, and there is no reason to merge something that is not ready.

  • The new sphinx_plot function is not documented nor tested.

Well, it is not even a function but a string.

The plot_pre_code string could be an import statement of a genuine doctested function, currently the code of the function is within a configuration file, instead we could have a shorter plot_pre_code in src/doc/common/conf.py and a genuine sphinx_plot function in src/sage/plot/misc.py (say).

And I do not see what you want me to test, the function returns no output.

For example, we could test the existence of the file and its timestamp (in case the image comes from an older build), its size (dimensions), the color of some pixels in a quite predictible image ? The plot directive seems not a sphinx standard, and i am not sure matplotlib will not change its behaviour, or remove it, rename it (e.g. if sphinx incorporate it with another name). Hence, there should perhaps be a way to report something if the drawing goes wrong.

What is less clear to me is how to trigger the test (only after a docbuild that used pictures).

It would be nice, but I have no idea how to do that you want of me.

I would like to clearly avoid some misunderstanding: i am not expecting some additional work from you (nor judging the quality of your code or whatever), only trying to contribute to a ticket that should add more features than problems within Sage.

Back to the main issue, John's current fix is a good step, but it does not solve the fact that when we run make to build Sage, the doc is built with pictures with no possibility to avoid it, and then we have to rebuild the doc again to remove them.

I see various points to make John's fix work better:

  • the way it is currently written, it is uselsss for the user to set the SPHINX_INCLUDE_PLOTS environment variable because it is overwritten by the builder.py script in any case. It could be better to use this variable if the user defined it, so that the user could avoid building the wrong documentation by typing export SPHINX_INCLUDE_PLOTS=no before doing a make. One possibility is to document this variable and to overwrite it only if --no-pix is explicitely stated.
  • Note that in Sage, environment variables usually get values yes/no instead of True/False, and their name start with SAGE_.
  • Instead of adding a new html-no-pix FORMAT for the docbuild command, it could be better to add a --no-pix OPTION (or perhaps --no-plot ?). See sage -docbuild for the documentation. This is more consistent with the existing --no-pdf-links or --no-colors options, but also allows to pass the --no-pix (or whatever) option in the Makefile calls via the SAGE_DOCBUILD_OPTS command.

comment:58 in reply to: ↑ 57 Changed 4 years ago by ncohen

Yop !

Not knowing how to fix this issue is not a sufficient condition to let the size of every Sage binary explode

As you will notice, the binary never exploded. There are <=10 pictures added by this branch, nothing more. Everything else is your imagination of what the future will be. And in that future, we still have time to write patches if we need to. So that never happened.

I was not claiming that you should fix it yourself, i was claiming that the ticket needs work, working on a ticket is a collaborative task, and there is no reason to merge something that is not ready.

Why didn't you add a commit ? In my understanding of what this ticket should do, what you wanted was an 'added feature'. This is why I thought it was not a problem to have this one reviewed first. I just want to add a new feature that we have been needing for a long time. We will have to deal with the problems that this will create in the future, it is true. But well, this is not a problem now and we can fix it whenever we like, so I did not think that it was a problem for this ticket.

This being said, John solved that as the manual can be built without pictures.

The plot_pre_code string could be an import statement of a genuine doctested function, currently the code of the function is within a configuration file, instead we could have a shorter plot_pre_code in src/doc/common/conf.py and a genuine sphinx_plot function in src/sage/plot/misc.py (say).

I do not understand that. plot_pre_code is a string variable that is expected by matplotlib's 'plot' directive for Sphinx. How would you give it anthing else than a string ?

And I do not see what you want me to test, the function returns no output.

For example, we could test the existence of the file and its timestamp (in case the image comes from an older build), its size (dimensions), the color of some pixels in a quite predictible image ?

The sphinx_draw function is meant to display a picture (this is what matplotlib intercepts), not to write it to a file. Writing it to a file is just a mean to display it "as it has to be displayed", and if anything must be doctested it should be that.

I have no idea what kind of things we could 'check' by opening the image file directly though O_o

The plot directive seems not a sphinx standard,

No, it comes from matplotlib

and i am not sure matplotlib will not change its behaviour, or remove it, rename it (e.g. if sphinx incorporate it with another name). Hence, there should perhaps be a way to report something if the drawing goes wrong.

I do not know how to check that. But that is similar to the fact that we have no way to check in the doctests that our .show() calls actually do something. If that function changed, no doctest would be broken I expect, and there are many doctests that call show.

Back to the main issue, John's current fix is a good step, but it does not solve the fact that when we run make to build Sage, the doc is built with pictures with no possibility to avoid it, and then we have to rebuild the doc again to remove them.

True, true. For me you want to solve a problem that does not exist yet, but this is clearly the kind of things we will neeed someday.

  • the way it is currently written, it is uselsss for the user to set the SPHINX_INCLUDE_PLOTS environment variable because it is overwritten by the builder.py script in any case.

Right now there is no user-side doc mentionning SPHINX_INCLUDE_PLOTS. This variable is set and used internally only, and that works fine. If you want to advertise this to users, then indeed the name may have to be changed.

  • Note that in Sage, environment variables usually get values yes/no instead of True/False, and their name start with SAGE_.

The same comment applies to that, I guess.

  • Instead of adding a new html-no-pix FORMAT for the docbuild command, it could be better to add a --no-pix OPTION (or perhaps --no-plot ?). See sage -docbuild for the documentation. This is more consistent with the existing --no-pdf-links or --no-colors options, but also allows to pass the --no-pix (or whatever) option in the Makefile calls via the SAGE_DOCBUILD_OPTS command.

If you prefer. Would you be ready to write a commit for all these changes that you request ? I do not understand why you see all these things as a reason to block this patch from being merged into Sage. I have less problems with it if you are willing to write yourself this added code, as it would not mean that this ticket is blocked until I implement what you ask.

Nathann

comment:59 Changed 4 years ago by git

  • Commit changed from d7ae73cdec62e1d461fb951ef962746ddbdfd1b7 to 6e51751b533c42f10f18f00dc7a01b4dacf0aa43

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

6e51751#17498: change "html-no-pix" to "--no-plot html"

comment:60 in reply to: ↑ 57 ; follow-up: Changed 4 years ago by jhpalmieri

Replying to tmonteil:

Back to the main issue, John's current fix is a good step, but it does not solve the fact that when we run make to build Sage, the doc is built with pictures with no possibility to avoid it, and then we have to rebuild the doc again to remove them.

No, you just run make doc-html-no-pix: this builds Sage and then the docs, but with no pictures.

I see various points to make John's fix work better:

  • the way it is currently written, it is uselsss for the user to set the SPHINX_INCLUDE_PLOTS environment variable

My goal wasn't to introduce an environment variable for the user to set, which is why I didn't document it in the installation guide. This style is common throughout builder.py. Various people have suggested, and I agree, that Sage has too many environment variables as it is, and we should instead control behavior using command-line flags, Makefile targets, etc. In this case, the environment variable (for internal use only) is a useful way to pass the information around.

  • Instead of adding a new html-no-pix FORMAT for the docbuild command, it could be better to add a --no-pix OPTION (or perhaps --no-plot ?). See sage -docbuild for the documentation. This is more consistent with the existing --no-pdf-links or --no-colors options, but also allows to pass the --no-pix (or whatever) option in the Makefile calls via the SAGE_DOCBUILD_OPTS command.

This sounds like a good idea. Shouldn't be hard to do. See the new commit.


New commits:

6e51751#17498: change "html-no-pix" to "--no-plot html"

comment:61 in reply to: ↑ 60 Changed 4 years ago by tmonteil

Replying to jhpalmieri:

Thanks for your commit, now we can do

export SAGE_DOCBUILD_OPTS+=' --no-plot'

and continue in a usual workflow.

Replying to tmonteil:

Back to the main issue, John's current fix is a good step, but it does not solve the fact that when we run make to build Sage, the doc is built with pictures with no possibility to avoid it, and then we have to rebuild the doc again to remove them.

No, you just run make doc-html-no-pix: this builds Sage and then the docs, but with no pictures.

My point was that many targets build docs and could not have plots turned off, which will require rebuilding doc afterwards. For example it would looks weird to add make ptestlong-no-pix target. Conversely, environment variables can be combined, this somehow avoids quadratic explosion of make targets.

My goal wasn't to introduce an environment variable for the user to set, which is why I didn't document it in the installation guide. This style is common throughout builder.py. Various people have suggested, and I agree, that Sage has too many environment variables as it is, and we should instead control behavior using command-line flags, Makefile targets, etc. In this case, the environment variable (for internal use only) is a useful way to pass the information around.

OK. So let us let the SAGE_SKIP_PLOT_DIRECTIVE undocumented for now and consider it internal only.

  • Instead of adding a new html-no-pix FORMAT for the docbuild command, it could be better to add a --no-pix OPTION (or perhaps --no-plot ?). See sage -docbuild for the documentation. This is more consistent with the existing --no-pdf-links or --no-colors options, but also allows to pass the --no-pix (or whatever) option in the Makefile calls via the SAGE_DOCBUILD_OPTS command.

This sounds like a good idea. Shouldn't be hard to do. See the new commit.

Thanks, it works well. And it solves the previous possible issue with make targets.

comment:62 Changed 4 years ago by git

  • Commit changed from 6e51751b533c42f10f18f00dc7a01b4dacf0aa43 to ce7fd1197e53f1fe8cb4ce23b5732a39a6ff22ce

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

0b37479#17498 : rename doc-html-no-pix make target to doc-html-no-plot
546bc79#17498 : add some doc on how to use -no-plot within SAGE_DOCBUILD_OPTS
ce7fd11#17498 : remove the broken "(Source code)" link above plots (will work on newer matplotlib)

comment:63 Changed 4 years ago by git

  • Commit changed from ce7fd1197e53f1fe8cb4ce23b5732a39a6ff22ce to 0998e30a8ce7b09dfeceb958718e4bb59d148c10

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

0998e30#17498 : update the "make targets" section in the installation doc

comment:64 follow-up: Changed 4 years ago by tmonteil

Minor fixes: added some doc on how to use --no-plot within SAGE_DOCBUILD_OPTS and renamed the doc-html-no-pix make target to doc-html-no-plot.

Also, currently, there is a broken (Source code) link above each image (and above nothing if plots are disabled). The plot_include_source variable to remove it is broken. Though plot_directive.py is easily patchable to let this latter work, i did some attempts with latest version of matplotlib that proposes a (working) plot_html_show_source_link variable. Hence the last commit sets this variable to True, so that the broken link will disapear once #17618 will be fixed.

Does this link removal (and the way to do it by waiting matplotlib upgrade) sounds good ?

comment:65 in reply to: ↑ 64 Changed 4 years ago by ncohen

Does this link removal (and the way to do it by waiting matplotlib upgrade) sounds good ?

+1 to that ! I had tried to remove it for a while and never found out how.

Nathann

comment:66 follow-up: Changed 4 years ago by tmonteil

  • Status changed from needs_work to needs_review

I am personally OK with the current branch. Should we add #17618 as a dependency to avoid broken (Source code) links above pictures (or non-pictures) ?

comment:67 in reply to: ↑ 66 ; follow-up: Changed 4 years ago by ncohen

I am personally OK with the current branch. Should we add #17618 as a dependency to avoid broken (Source code) links above pictures (or non-pictures) ?

If we merge this first, won't those link disappear anyway when #17618 is merged?

Nathann

comment:68 in reply to: ↑ 67 ; follow-up: Changed 4 years ago by tmonteil

Replying to ncohen:

If we merge this first, won't those link disappear anyway when #17618 is merged?

Yes it should.

comment:69 in reply to: ↑ 68 Changed 4 years ago by ncohen

Yes it should.

Cool. So you can do what you want I guess (i.e. add it as a dependency of not).

comment:70 Changed 4 years ago by ncohen

Ping ?...

comment:71 Changed 4 years ago by jhpalmieri

  • Reviewers changed from Nathann Cohen, John Palmieri to Nathann Cohen, John Palmieri, Thierry Monteil
  • Status changed from needs_review to positive_review

comment:72 Changed 4 years ago by ncohen

Thanks !

Nathann

comment:73 Changed 4 years ago by vbraun

  • Branch changed from public/17498 to 0998e30a8ce7b09dfeceb958718e4bb59d148c10
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:74 Changed 4 years ago by tmonteil

  • Authors changed from Nathann Cohen, John Palmieri to Nathann Cohen, John Palmieri, Thierry Monteil
  • Commit 0998e30a8ce7b09dfeceb958718e4bb59d148c10 deleted

Sorry for noticing that only now, there was a trailing 'html-no-pix' format from an earlier commit that was not cleaned once we moved to '--no-plot' option, see #17812 as a follow-up.

Note: See TracTickets for help on using tickets.