#17498: Pictures in the doc through ".. plot::" directive
-------------------------------------+-------------------------------------
Reporter: ncohen | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.5
Component: documentation | Resolution:
Keywords: | Merged in:
Authors: Nathann Cohen, | Reviewers: Nathann Cohen, John
John Palmieri | Palmieri
Report Upstream: N/A | Work issues:
Branch: public/17498 | Commit:
Dependencies: #17508 | d7ae73cdec62e1d461fb951ef962746ddbdfd1b7
| Stopgaps:
-------------------------------------+-------------------------------------
Comment (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
--
Ticket URL: <http://trac.sagemath.org/ticket/17498#comment:58>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
