Opened 6 years ago

Closed 4 months ago

Last modified 2 months ago

#20343 closed enhancement (fixed)

Add file `sage/misc/latex_standalone.py` and class `TikzPicture`

Reported by: slabbe Owned by:
Priority: major Milestone: sage-9.6
Component: misc Keywords:
Cc: dkrenn, jipilab, slelievre, chapoton Merged in:
Authors: Sébastien Labbé Reviewers: Jean-Philippe Labbé, Xavier Caruso, Vincent Delecroix, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 5c9b41c (Commits, GitHub, GitLab) Commit:
Dependencies: #32650, #33005 Stopgaps:

Status badges

Description (last modified by slelievre)

The goal of this ticket is to import the TikzPicture module from the optional SageMath package slabbe into SageMath. This module has been created in 2016 and has evolved to a stable state and it is mature enough to go into SageMath now.

Here is a demo inside a Jupyter notebook (notice that the import line was since changed to from sage.misc.latex_standalone import TikzPicture):

More examples:

sage: from sage.misc.latex_standalone import TikzPicture
sage: V = [[1,0,1],[1,0,0],[1,1,0],[0,0,-1],[0,1,0],[-1,0,0],[0,1,1],[0,0,1],[0,-1,0]]
sage: P = Polyhedron(vertices=V).polar()
sage: s = P.projection().tikz([674,108,-731],112)
sage: t = TikzPicture(s)
sage: t.pdf()             # opens the pdf in a viewer                                                        
'/home/slabbe/.sage/temp/miami/5032/tikz_gvem9vu8.pdf'
sage: t.pdf('image.pdf')  # when a filename is provided, it doesn't open in a viewer                                            
'/home/slabbe/image.pdf'

It may create the image into png format (translation pdf -> png made with convert from imagemagick) or svg format (translation pdf -> svg made with pdf2svg):

sage: path_to_file = t.png()                                              
sage: path_to_file = t.svg()
sage: path_to_file = t.tex()

In a Jupyter notebook, this shows the picture below the cell (using rich repr):

sage: t

In the terminal, this shows the header and tail of the tikzpicture code:

sage: t                                                                                                    
\documentclass[tikz]{standalone}
\usepackage{amsmath}
\begin{document}
\begin{tikzpicture}%
	[x={(0.249656cm, -0.577639cm)},
	y={(0.777700cm, -0.358578cm)},
	z={(-0.576936cm, -0.733318cm)},
	scale=1.000000,
---
91 lines not printed (5169 characters in total).
Use print to see the full content.
---
\node[vertex] at (0.00000, -1.00000, 0.00000)     {};
\node[vertex] at (-0.50000, -0.50000, -0.50000)     {};
%%
%%
\end{tikzpicture}
\end{document}

Use print to see the full content:

sage: print(t) 

A second example shows that it is faster than view and better (avoids to crop the vertices):

sage: g = graphs.PetersenGraph()
sage: %time _ = TikzPicture(latex(g), standalone_options=["border=4mm"], usepackage=['tkz-graph']).pdf()
CPU times: user 3.52 ms, sys: 12.7 ms, total: 16.2 ms
Wall time: 2.24 s     

compared to using view (which crops the vertices and creates two pages pdf on mac):

sage: %time view(g, tightpage=True)               
CPU times: user 126 ms, sys: 85 ms, total: 211 ms 
Wall time: 6.06 s

Also, if dot2tex and graphviz available, one may directly construct a tikzpicture from a sagemath graph:

sage: t = TikzPicture.from_graph(g, prog='dot')  # optional: dot2tex

The tikzpicture.py was developped within optional package slabbe since 2015. Its interface should be stable enough to go into sage now.

Meanwhile any user can use this tikzpicture module by installing the package:

sage -pip install slabbe

and

sage: from slabbe import TikzPicture

An example of usage of this module can be found at this url:

associated to the preprint

published in Journal of Modern Dynamics at

Attachments (1)

tikz-jupyter-demo.png (146.7 KB) - added by slabbe 7 months ago.

Download all attachments as: .zip

Change History (172)

comment:1 Changed 6 years ago by slabbe

  • Description modified (diff)

comment:2 Changed 6 years ago by slabbe

  • Branch set to u/slabbe/20343
  • Commit set to 476b574c4198115e6e98e3396ed32f4b95077c35
  • Status changed from new to needs_review

New commits:

476b57420343: Adding tikzpicture.py in sage/misc

comment:3 Changed 6 years ago by slabbe

  • Description modified (diff)

comment:4 Changed 6 years ago by slabbe

  • Summary changed from Adding sage/misc/tikz_picture.py to Adding sage/misc/tikzpicture.py

comment:5 follow-up: Changed 6 years ago by dkrenn

  • Cc dkrenn added

A comment:

t = TikzPicture(latex(transducers.GrayCode()))
t.pdf()

does not work as the preamble is not set correctly. However, there is

from sage.combinat.finite_state_machine import setup_latex_preamble
setup_latex_preamble()

(as well as the corresponding function in sage.graphs.graph_latex) to set this correctly, but it seems to be ignored by TikzPicture.

In contrast,

view(transducers.GrayCode())

works, but I have no idea if setup_latex_preamble is called (not needed to be done manually) or if tikz is included anyways.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by slabbe

Replying to dkrenn:

A comment:

t = TikzPicture(latex(transducers.GrayCode()))
t.pdf()

does not work as the preamble is not set correctly.

As it is now, the user must set the libraries. This should work:

t = TikzPicture(latex(transducers.GrayCode()), tikzlibraries=['automata'])
t.pdf()

I am still not sure about the proper way to choose the arguments for the __init__ function and their especially their default values. Should the arguments be empty by default? equal to the current sage latex preamble? or full of a big load of libraries?

comment:7 in reply to: ↑ 6 Changed 6 years ago by dkrenn

Replying to slabbe:

Replying to dkrenn:

A comment:

t = TikzPicture(latex(transducers.GrayCode()))
t.pdf()

does not work as the preamble is not set correctly.

As it is now, the user must set the libraries. This should work:

t = TikzPicture(latex(transducers.GrayCode()), tikzlibraries=['automata'])
t.pdf()

Works, thanks.

I am still not sure about the proper way to choose the arguments for the __init__ function and their especially their default values. Should the arguments be empty by default? equal to the current sage latex preamble? or full of a big load of libraries?

Without much thinking, I vote for: The second, equal to the current sage latex preamble.

comment:8 Changed 6 years ago by git

  • Commit changed from 476b574c4198115e6e98e3396ed32f4b95077c35 to 4ae7f63de1517e4c483e500757989b6efde5b785

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

751209120343: Adding tikzpicture.py in sage/misc
4ae7f6320343: improving the default argument choices for TikzPicture

comment:9 Changed 6 years ago by slabbe

  • Description modified (diff)

comment:10 Changed 6 years ago by slabbe

  • Branch u/slabbe/20343 deleted
  • Commit 4ae7f63de1517e4c483e500757989b6efde5b785 deleted
  • Description modified (diff)

My module has evolved in my package slabbe, therefore the branch here is not up to date anymore. Also, I intend to make it more general during the next year allowing any content not only tikzpicture code. Therefore, I deleted the branch posted here.

comment:11 Changed 6 years ago by slabbe

  • Description modified (diff)
  • Status changed from needs_review to needs_work

Changing status to needs work.

comment:12 Changed 5 years ago by jipilab

  • Cc jipilab added
  • Reviewers set to Jean-Philippe Labbé

As a follow-up ticket to make use of this is #22506.

comment:13 Changed 16 months ago by caruso

  • Branch set to u/caruso/tikzpicture

comment:14 Changed 16 months ago by caruso

  • Commit set to 88c048c2a33618220a586f708273e41fd803f6bc

I'll be happy to revive this ticket.


New commits:

88c048cimport tikz_picture.py from slabbe gitlab repository

comment:15 Changed 16 months ago by git

  • Commit changed from 88c048c2a33618220a586f708273e41fd803f6bc to 2917f4b60238afcb4f097f3a256f734141aabe02

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

2917f4bdoctests pass

comment:16 Changed 16 months ago by git

  • Commit changed from 2917f4b60238afcb4f097f3a256f734141aabe02 to 8cfc83431a25394be8b0dbc890d0dc0f72f54f56

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

8cfc834typos

comment:17 Changed 16 months ago by caruso

I'm not convinced by all the methods from_dot_string, from_graph, etc. in the class TikzPicture. I would say that this code should instead be in the classes implementing the object we want to display. E.g. it could be a method _tikz_, just as _latex_, and TikzPicture just calls it.

What's your opinion?

comment:18 Changed 16 months ago by slabbe

There is no class for dot strings. Graphs already have a _latex_ method which returns a tikz picture string. It is what is used with for instance view(G). The from_graph class method really is using dot2tex + graphviz which is not the same layout sometimes. It is true that maybe adding a _tikz_ would avoid to replace the current behavior of _latex_. But then, having two different behaviors will be non pedagogical (how to know there is another one). Also, changing the default behavior will lead to problems...

I think it will be easier to add the new module as is and later on, in other tickets, add such _tikz_ methods in other parts of sage where it makes sense.

comment:19 Changed 16 months ago by caruso

  • Status changed from needs_work to needs_review

OK. Let's keep it for another ticket.

comment:20 Changed 16 months ago by caruso

  • Status changed from needs_review to positive_review

comment:21 Changed 16 months ago by caruso

  • Milestone changed from sage-7.2 to sage-9.4
  • Reviewers changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Xavier Caruso

comment:22 Changed 16 months ago by chapoton

please no "from future import ..."

Returns should be Return

comment:23 Changed 16 months ago by caruso

  • Status changed from positive_review to needs_work

Also if pdflatex is not installed on the machine, some doctests fail. I don't know if this should be fixed but I think so because otherwise patchbots complain.

comment:24 Changed 16 months ago by slabbe

  • Branch changed from u/caruso/tikzpicture to u/slabbe/20343
  • Commit changed from 8cfc83431a25394be8b0dbc890d0dc0f72f54f56 to 665db0b0dd19254278d3cd74d96423a617781223

New commits:

665db0b20343: removed from __future__ imports

comment:25 Changed 16 months ago by git

  • Commit changed from 665db0b0dd19254278d3cd74d96423a617781223 to 0022eb2e8333597e2f4004e6b3a2d9363713df88

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

0022eb220343: Returns -> Return

comment:26 Changed 16 months ago by git

  • Commit changed from 0022eb2e8333597e2f4004e6b3a2d9363713df88 to 03aa1bccb5640263e1d23867ee742b1ce9ac3ae0

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

03aa1bc20343: adding few optional tags

comment:27 Changed 16 months ago by slabbe

  • Status changed from needs_work to needs_review

I added a bunch of #optional latex and #optional imagemagick tags. That should fix most of the issues.

I also added one # optional pdf2svg for which I am not sure about. I don't know if the doctest framework know about this tag. I change to needs review to check that on the patchbot.

comment:28 Changed 16 months ago by vdelecroix

Why do you make it part of Sage and not an independent Python module? These could easily be declared as optional/standard packages afterwards.

comment:29 follow-up: Changed 16 months ago by slabbe

Yes, I agree with that option too. Lately, I was thinking more about this option.

But, the module also depends on many things that are in Sage which would make this module quite unusable outside of sage as it is currently. For example:

To check if Graphviz is there:

from sage.features.graphviz import Graphviz

To check if pdflatex, convert and other programs are available:

from sage.misc.latex import have_pdflatex, have_convert, have_program

To load the latex macros from sage:

from sage.misc.latex import _Latex_prefs
from sage.misc.latex_macros import sage_latex_macros

To create a temporary file inside the sage folder of temporary files:

from sage.misc.temporary_file import tmp_filename

To open viewers:

from sage.misc.viewer import browser
from sage.misc.viewer import pdf_viewer
from sage.misc.viewer import png_viewer

To check whether we are in the JupyterNotebook or not:

from sage.repl.rich_output.backend_ipython import BackendIPythonNotebook
from sage.repl.rich_output.buffer import OutputBuffer
Last edited 16 months ago by slabbe (previous) (diff)

comment:30 Changed 16 months ago by git

  • Commit changed from 03aa1bccb5640263e1d23867ee742b1ce9ac3ae0 to 7d590342121285b6fea577b0defc65aece99258e

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

7d5903420343: adding few optional latex tags

comment:31 in reply to: ↑ 29 Changed 14 months ago by vdelecroix

Replying to slabbe:

Yes, I agree with that option too. Lately, I was thinking more about this option.

The main advantage with that option is that the package can change versions independently of sage.

But, the module also depends on many things that are in Sage which would make this module quite unusable outside of sage as it is currently. For example:

This would be a Python module that depends on sage. There is nothing bad with that situation. And this is not the only one.

comment:32 Changed 14 months ago by slabbe

I am thinking about what to do now, either work on this ticket or create a new package. If I create a sagemath-tikzpicture package that imports many parts of sagemath, I will be responsible for fixing the package each time sagemath changes something that I import. On the other hand, if that modules goes into sagemath, then, it will become the responsability of the person which changes the dependencies to also fix the tikzpicture module. I agree with the idea of modularization, but for the above reason, I would prefer to make it go into sage. What do you think?

Last edited 14 months ago by slabbe (previous) (diff)

comment:33 Changed 13 months ago by caruso

I prefer to have this directly in sage since I think that there are many other places in sage where it would be great to have a tikz output.

comment:34 Changed 13 months ago by mkoeppe

Because this module does not depend on any external libraries, there is nothing gained in the direction of modularization by making this a separate distribution package.

But perhaps you can find a better place for it than sage.misc? How about sage.typeset or sage.plot?

comment:35 Changed 13 months ago by git

  • Commit changed from 7d590342121285b6fea577b0defc65aece99258e to 9f9f5ec90bd47bae4ffe1a3e838f1cad229619a2

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

d84b6a3Merge branch 'u/slabbe/20343' of trac.sagemath.org:sage into 20343
ee56d32adding optional tag latex to one doctest
269268520343:avoid use of doctest continuation since it is not one
2acfe6120343: misc/tikz_picture.py -> plot/tikzpicture.py
388a5f8from misc.tikz_picture -> from plot.tikzpicture
b04ea0dupdated authors and copyright
9f9f5ec20343: redoing recent changes by Vincent Delecroix in slabbe package (check_call -> run); improved header documentation

comment:36 Changed 13 months ago by slabbe

  • Description modified (diff)
  • Summary changed from Adding sage/misc/tikzpicture.py to Adding sage/plot/tikzpicture.py

comment:37 Changed 13 months ago by slabbe

  • Description modified (diff)

comment:38 Changed 13 months ago by slabbe

  • Description modified (diff)

comment:39 Changed 13 months ago by slabbe

  • Description modified (diff)

comment:40 Changed 13 months ago by slabbe

  • Description modified (diff)

comment:41 Changed 13 months ago by slabbe

I think we are close to it. The doctests work on my machine:

sage -t --optional=sage,optional,external --long --show-skipped tikzpicture.py 

gives

Using --optional=4ti2,ccache,cryptominisat,debian,dot2tex,e_antic,external,fricas,glucose,latte_int,lidia,normaliz,notedown,pandoc_attributes,pip,pycosat,pynormaliz,rst2ipynb,sage,sage_numerical_backends_coin,sage_spkg
External software to be detected: cplex,ffmpeg,graphviz,gurobi,imagemagick,internet,latex,macaulay2,magma,maple,mathematica,matlab,octave,pandoc,rubiks,scilab
Doctesting 1 file.
sage -t --long --random-seed=0 tikzpicture.py
    19 not tested tests not run
    1 pdf2svg test not run
    0 tests not run because we ran out of time
    [161 tests, 15.43 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 15.6 seconds
    cpu time: 8.2 seconds
    cumulative wall time: 15.4 seconds
External software detected for doctesting: imagemagick,latex

TODO:

  • Let's see what the patchbot says.
  • The doctest tagged with pdf2svg is not recognized by the --optional=external. Maybe something needs to be done on the detection of external packages.
  • One issue is that the current _rich_repr_ always uses png instead of svg in the Jupyter Notebook even if I define 'svg' before 'png' in the prefer_vector tuple. I think that issue can be dealt in a later ticket.

That's it! Needs review!

comment:42 Changed 13 months ago by mkoeppe

  • Cc slelievre added

This ad-hoc system package advice

+        if not have_program('pdf2svg'):
+            raise RuntimeError("pdf2svg does not seem to be installed. "
+                    "Install it for example with ``brew install pdf2svg``"
+                    " or ``apt-get install pdf2svg``.")
+

should be replaced by a Feature and an spkg pdf2svg with distros/ information from https://repology.org/project/pdf2svg/versions

comment:43 Changed 13 months ago by slabbe

Ok, I will add a Feature for pdf2svg.

Also, patchbot says this:

src/sage/plot/tikzpicture.py:98:1 'subprocess.CalledProcessError' imported but unused
found 1 pyflakes errors in the modified files

Another TODO is to allow to get some verbosity useful when compilation of the picture breaks. I am writting it here not to forget.

I will work on that soon or later.

comment:44 Changed 13 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:45 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:46 Changed 9 months ago by slabbe

  • Dependencies set to #32650

comment:47 Changed 9 months ago by git

  • Commit changed from 9f9f5ec90bd47bae4ffe1a3e838f1cad229619a2 to 80151c25e7aa040973b6f6268f49ddc3b521cfd7

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

a96e94d32650: Creating feature pdf2svg
870d46132650: Creating features latex, xelatex, pdflatex, lualatex
385569e32650: adding latex, xelatex, pdflatex, lualatex, pdf2svg, dvipng to doctest/external.py
f3291d832650: using features instead of have_program in misc/latex.py
d45599332650: using features in plot/animate.py
cf0209432650: using features in plot/graphics.py and plot/multigraphics.py
28f1f4a32650: deprecating have_* functions in misc/latex.py in favor of features
5d9414d32650: .. note -> .. NOTE
6d21ec7Merge branch #20343 onto #32650
80151c220343: using features instead of have_program, have_pdflatex, have_convert

comment:48 Changed 9 months ago by slabbe

I rebased on top of current #32650.

Current TODOS are:

  • verbosity when compilation breaks
  • add distro info stuff in pdf2svg (should be done in #32650)
  • improve rich repr to use svg not always png

comment:49 Changed 9 months ago by git

  • Commit changed from 80151c25e7aa040973b6f6268f49ddc3b521cfd7 to 997b8f1a58afe1b8eae04a030ec5b769fa8463b7

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

997b8f120343: print the log when pdflatex fails compile the tikz image

comment:50 follow-up: Changed 9 months ago by slabbe

  • Status changed from needs_work to needs_review

With the recent commit and subsequent work done in #32650, the remainings TODOs are:

  • improve rich repr to use svg not always png

and as I suggested before, this one could be dealt in another ticket as it is still functional with png's in the Jupyter notebook.

Needs review!

comment:51 Changed 9 months ago by slabbe

  • Description modified (diff)

comment:52 Changed 9 months ago by slabbe

  • Description modified (diff)

comment:53 Changed 9 months ago by slabbe

  • Description modified (diff)

comment:54 follow-up: Changed 9 months ago by mkoeppe

In a similar direction as comment:17, I think that the from_graph... and from_poset class methods should rather be changed to methods of Graph and Poset.

comment:55 Changed 9 months ago by mkoeppe

comment:56 Changed 8 months ago by git

  • Commit changed from 997b8f1a58afe1b8eae04a030ec5b769fa8463b7 to 6a085cfdf1c930e4322a43d9d77356c2128ed913

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

f9c949220343: few small improvements
6a085cf20343: improving first example shown in documentation

comment:57 Changed 8 months ago by slabbe

I added two commits to do few improvements.

comment:58 in reply to: ↑ 54 ; follow-up: Changed 8 months ago by slabbe

Replying to mkoeppe:

In a similar direction as comment:17, I think that the from_graph... and from_poset class methods should rather be changed to methods of Graph and Poset.

If you want to go in that direction, we need to have a design discussion before spending time changing the graph code. As you know,

sage: g = graphs.PetersenGraph()
sage: hasattr(g, '_latex_')
True

which returns an object of type:

sage: type(latex(g))
<class 'sage.misc.latex.LatexExpr'>

This latex expression starts with \begin{tikzpicture} and ends with \end{tikzpicture} which also depends on \usepackage{tkz-graph}:

sage: latex(g)
\begin{tikzpicture}                                                                                    
\definecolor{cv0}{rgb}{0.0,0.0,0.0}                                                                    
\definecolor{cfv0}{rgb}{1.0,1.0,1.0}                                                                   
...
\Edge[lw=0.1cm,style={color=cv6v9,},](v6)(v9)
\Edge[lw=0.1cm,style={color=cv7v9,},](v7)(v9)
%
\end{tikzpicture}

Notice that this output can be provided to TikzPicture with no problem with t = TikzPicture(latex(g), usepackage=['tkz-graph']) and t.pdf() works etc.

I don't think here we want to replace the output of g._latex_ by an object of type TikzPicture since a such change would break a lot of code.

An option is to create a new method def tikz(self) instead. I can do something like that. It could return the TikzPicture(latex(g), usepackage=['tkz-graph']) by default and if one asks, it could use the graphviz + dot2tex output instead. But, I was thinking of doing this in another ticket as a follow up, as this will open discussions that will just make longer before this ticket can go in.

Or maybe you would rather like to remove the from_graph and the from_poset methods to avoid to have to maintain them in the future? Well, currently, the from_graph also allows to merge the multiple edges which is something that would not go in the tikz method of a graph. Also, keeping the from_graph would make the passage from this module to sage easier for me because I have been using this method over and over and over in my research code.

comment:59 Changed 8 months ago by slabbe

I created ticket #33002 for the addition of a method to return an object of type TikzPicture in graph, posets and polyhedron code.

comment:60 Changed 8 months ago by git

  • Commit changed from 6a085cfdf1c930e4322a43d9d77356c2128ed913 to d6d473a1400059b385104ed4220097fde906e82f

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

d6d473a20343: fixing a bug, png output was not working

comment:61 in reply to: ↑ 50 Changed 8 months ago by slabbe

Replying to slabbe:

With the recent commit and subsequent work done in #32650, the remainings TODOs are:

  • improve rich repr to use svg not always png

and as I suggested before, this one could be dealt in another ticket as it is still functional with png's in the Jupyter notebook.

Also, I want to say that I believe that the sage.repl code is broken with respect to showing svg files.

# Jupyter cell 1
g = graphs.PetersenGraph()
from sage.plot.tikzpicture import TikzPicture
t = TikzPicture(latex(g), usepackage=['tkz-graph'])
t # works: it shows a png
# Jupyter cell 2
from sage.repl.rich_output import get_display_manager
dm = get_display_manager()
assert dm.preferences.graphics is None
dm.preferences.graphics = 'raster'
t  # works: it shows a png
# Jupyter cell 3
dm.preferences.graphics = 'vector'
t   # broken: it does not show anything

The following shows that the Jupyter notebook is able to handle a svg correctly:

# Jupyter cell 4
t.svg('a.svg')
from IPython.core.display import SVG
SVG(filename='a.svg')

I suggest to fix the issue with the showing of svg in another ticket.

comment:62 Changed 8 months ago by git

  • Commit changed from d6d473a1400059b385104ed4220097fde906e82f to 2da56a8cb87d3e2de789bbfd08375baee36009fd

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

2da56a820343: using pdftocairo instead of pdf2svg

comment:63 Changed 8 months ago by slabbe

  • Dependencies changed from #32650 to #32650, #33005

comment:64 in reply to: ↑ 58 Changed 8 months ago by mkoeppe

Replying to slabbe:

Replying to mkoeppe:

In a similar direction as comment:17, I think that the from_graph... and from_poset class methods should rather be changed to methods of Graph and Poset.

I don't think here we want to replace the output of g._latex_ by an object of type TikzPicture since a such change would break a lot of code.

I agree

An option is to create a new method def tikz(self) instead. I can do something like that.

Yes, this is what I meant

comment:65 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:66 Changed 7 months ago by git

  • Commit changed from 2da56a8cb87d3e2de789bbfd08375baee36009fd to 90817776abb4c1e1ed21ac6af983b337330d4b2c

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

d4ca003from misc.tikz_picture -> from plot.tikzpicture
42011d6updated authors and copyright
6894a5b20343: redoing recent changes by Vincent Delecroix in slabbe package (check_call -> run); improved header documentation
2c0792520343: using features instead of have_program, have_pdflatex, have_convert
83b917320343: print the log when pdflatex fails compile the tikz image
b26bc3f20343: few small improvements
ae7b9bf20343: improving first example shown in documentation
9f91d2220343: fixing a bug, png output was not working
f39e07a20343: using pdftocairo instead of pdf2svg
908177720343:fixing 2 doctests + wrong trailing colons

comment:67 Changed 7 months ago by slabbe

There was a syntax error in the file due to the previous commit. I fixed that.

I also rebased the branch on top of 9.5.beta9.

comment:68 Changed 7 months ago by git

  • Commit changed from 90817776abb4c1e1ed21ac6af983b337330d4b2c to a78e9ce673d3df77ca008af4c6e73490ff3d7ce7

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

b2388a120343: change default from pdf2svg to pdftocairo
a78e9ce20343: provide log if an error occurs in method png

comment:69 Changed 7 months ago by slabbe

I confirm it works on my side:

$ sage -t tikzpicture.py 
[...]
Doctesting 1 file.
sage -t --random-seed=33963620693510224516081799230454604818 tikzpicture.py
    [158 tests, 4.59 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Features detected for doctesting: 

and

$ sage -t --show-skipped --long --optional=sage,optional,external tikzpicture.py 
[...]
Doctesting 1 file.
sage -t --long --random-seed=204572532264437349038334999704478431314 tikzpicture.py
    24 not tested tests not run
    2 pdftocairo tests not run
    0 tests not run because we ran out of time
    [188 tests, 15.97 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Features detected for doctesting: imagemagick,latex,pdf2svg

Changed 7 months ago by slabbe

comment:70 Changed 7 months ago by slabbe

  • Description modified (diff)

Updating the description: adding a screenshot of the behavior in Jupyter, hoping it motivates someone to review the ticket.

comment:71 Changed 7 months ago by slabbe

  • Description modified (diff)

comment:72 Changed 7 months ago by slabbe

  • Description modified (diff)

comment:73 Changed 7 months ago by slabbe

  • Description modified (diff)

comment:74 Changed 7 months ago by vdelecroix

Why this default usepackage=['amsmath']?

comment:75 Changed 7 months ago by vdelecroix

Moreover, you should never have a list as a default argument which is plugged as an attribute

sage: class A():
....:     def __init__(self, a=[]):
....:         self.a = a
sage: x = A()
sage: x.a.append(2)
sage: y = A()
sage: y.a
[2]

comment:76 Changed 7 months ago by git

  • Commit changed from a78e9ce673d3df77ca008af4c6e73490ff3d7ce7 to fc47455b050b1a21244e6ab00ef3b88de8bf4d67

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

a3dec9220343: change default value of usepackage to None
fc4745520343: use shell=False instead

comment:77 follow-up: Changed 7 months ago by slabbe

You are right. I also avoided the usage of shell=True for security reasons, see https://docs.python.org/3/library/subprocess.html#security-considerations

Doing so, the cmd needs to be a list and not a string. This is better also because it allows filenames to contain spaces: "Providing a sequence of arguments is generally preferred, as it allows the module to take care of any required escaping and quoting of arguments (e.g. to permit spaces in file names)", see https://docs.python.org/3/library/subprocess.html#frequently-used-arguments

Last edited 7 months ago by slabbe (previous) (diff)

comment:78 in reply to: ↑ 77 ; follow-up: Changed 7 months ago by vdelecroix

Replying to slabbe:

[...] This is better also because it allows filenames to contain spaces [...]

If 'm y p i c t u r e.pdf' is an explicitely supported filename, it would make sense to mention it and test it. If it is an obscure feature, nevermind.

comment:79 Changed 7 months ago by git

  • Commit changed from fc47455b050b1a21244e6ab00ef3b88de8bf4d67 to ab3ec0299f903dc6cd5d25bf4cba0eb2aa4e47a1

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

ab3ec0220343: add doctest showing spaces in filename

comment:80 in reply to: ↑ 78 Changed 7 months ago by slabbe

Replying to vdelecroix:

If 'm y p i c t u r e.pdf' is an explicitely supported filename, it would make sense to mention it and test it. If it is an obscure feature, nevermind.

Yes, why not. I added a doctest confirming it works.

I also added a missing long doctest tag.

Needs review!

comment:81 Changed 7 months ago by slabbe

  • Milestone changed from sage-9.6 to sage-9.5

There is a time window right now for this ticket to get into sage-9.5 maybe within the first rc.

The patchbots are green. I will be available in the next days to fix quickly any remaining issues.

comment:82 Changed 7 months ago by vdelecroix

  • Reviewers changed from Jean-Philippe Labbé, Xavier Caruso to Jean-Philippe Labbé, Xavier Caruso, Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:83 Changed 7 months ago by slabbe

Thank you!! I was holding by breathe since 6 years!

I will add a nice section in the release notes if it makes it in 9.5.

comment:84 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:85 Changed 6 months ago by vbraun

  • Status changed from positive_review to needs_work
**********************************************************************
File "src/sage/plot/tikzpicture.py", line 73, in sage.plot.tikzpicture
Failed example:
    _ = t.png(view=False)      # long time (2s) # optional imagemagick
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.tikzpicture[14]>", line 1, in <module>
        _ = t.png(view=False)      # long time (2s) # optional imagemagick
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/plot/tikzpicture.py", line 505, in png
        _filename_pdf = self.pdf(filename=None, view=False)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/plot/tikzpicture.py", line 443, in pdf
        result.check_returncode()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/subprocess.py", line 460, in check_returncode
        raise CalledProcessError(self.returncode, self.args, self.stdout,
    subprocess.CalledProcessError: Command '['lualatex', '-interaction=nonstopmode', 'tikz_544oxd7o.tex']' returned non-zero exit status 1.
**********************************************************************
File "src/sage/plot/tikzpicture.py", line 494, in sage.plot.tikzpicture.StandaloneTex.png
Failed example:
    path_to_file = t.png(filename) # long time (2s)   # optional imagemagick
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.tikzpicture.StandaloneTex.png[7]>", line 1, in <module>
        path_to_file = t.png(filename) # long time (2s)   # optional imagemagick
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/plot/tikzpicture.py", line 505, in png
        _filename_pdf = self.pdf(filename=None, view=False)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/plot/tikzpicture.py", line 443, in pdf
        result.check_returncode()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/subprocess.py", line 460, in check_returncode
        raise CalledProcessError(self.returncode, self.args, self.stdout,
    subprocess.CalledProcessError: Command '['lualatex', '-interaction=nonstopmode', 'tikz_ikxvujw2.tex']' returned non-zero exit status 1.
**********************************************************************
File "src/sage/plot/tikzpicture.py", line 495, in sage.plot.tikzpicture.StandaloneTex.png
Failed example:
    path_to_file[-4:]              # long time (fast) # optional imagemagick
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.tikzpicture.StandaloneTex.png[8]>", line 1, in <module>
        path_to_file[-Integer(4):]              # long time (fast) # optional imagemagick
    NameError: name 'path_to_file' is not defined
**********************************************************************
File "src/sage/plot/tikzpicture.py", line 579, in sage.plot.tikzpicture.StandaloneTex.svg
Failed example:
    path_to_file = t.svg(filename, program='pdftocairo') # long time (2s)   # optional pdftocairo
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.tikzpicture.StandaloneTex.svg[7]>", line 1, in <module>
        path_to_file = t.svg(filename, program='pdftocairo') # long time (2s)   # optional pdftocairo
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/plot/tikzpicture.py", line 588, in svg
        _filename_pdf = self.pdf(filename=None, view=False)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/plot/tikzpicture.py", line 443, in pdf
        result.check_returncode()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/subprocess.py", line 460, in check_returncode
        raise CalledProcessError(self.returncode, self.args, self.stdout,
    subprocess.CalledProcessError: Command '['lualatex', '-interaction=nonstopmode', 'tikz_o2ho82xz.tex']' returned non-zero exit status 1.
**********************************************************************
File "src/sage/plot/tikzpicture.py", line 580, in sage.plot.tikzpicture.StandaloneTex.svg
Failed example:
    path_to_file[-4:]                                    # long time (fast) # optional pdftocairo
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.tikzpicture.StandaloneTex.svg[8]>", line 1, in <module>
        path_to_file[-Integer(4):]                                    # long time (fast) # optional pdftocairo
    NameError: name 'path_to_file' is not defined
**********************************************************************

comment:86 Changed 6 months ago by slabbe

Indeed, the failures show that when latex is not installed, there are missing optional latex tags. Sorry Volker.

comment:87 Changed 6 months ago by git

  • Commit changed from ab3ec0299f903dc6cd5d25bf4cba0eb2aa4e47a1 to 4b5bd661c86a07eab7ebef50bafd4d4822d4730f

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

c8c9cb120343: improving first example shown in documentation
3aae6c020343: fixing a bug, png output was not working
65ab05b20343: using pdftocairo instead of pdf2svg
cd8e60420343:fixing 2 doctests + wrong trailing colons
0790c8520343: change default from pdf2svg to pdftocairo
aef6bd420343: provide log if an error occurs in method png
d4695c020343: change default value of usepackage to None
d05788620343: use shell=False instead
a93377420343: add doctest showing spaces in filename
4b5bd6620343: adding missing optional latex tag

comment:88 Changed 6 months ago by slabbe

Added the missing latex tag. Also rebased on 9.6.beta1.

comment:89 Changed 6 months ago by slabbe

  • Status changed from needs_work to needs_review

comment:90 follow-up: Changed 6 months ago by vdelecroix

Why not using else here

+        if program == 'pdflatex':
+            pdflatex().require()
+        elif program == 'lualatex':
+            lualatex().require()
+        elif program not in ['pdflatex','lualatex']:
+            raise ValueError("program(={}) should be pdflatex or lualatex".format(program))

comment:91 follow-up: Changed 6 months ago by vdelecroix

Why do you prefix filename_png, filename_pdf, etc with an additional underscore, ie _filename_png, _filename_pdf?

comment:92 Changed 6 months ago by git

  • Commit changed from 4b5bd661c86a07eab7ebef50bafd4d4822d4730f to f1e4728d00efd130a0688ac79e616bffbae0c539

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

f1e472820343: using else instead of elif

comment:93 in reply to: ↑ 90 Changed 6 months ago by slabbe

Replying to vdelecroix:

Why not using else here

You are right. I don't know why I did not use else. I added a commit to fix that.

comment:94 in reply to: ↑ 91 ; follow-up: Changed 6 months ago by slabbe

Replying to vdelecroix:

Why do you prefix filename_png, filename_pdf, etc with an additional underscore, ie _filename_png, _filename_pdf?

The reason is to distinguish the variable filename (input given from the user) from the variable _filename (name of a temporary file created by the method). See the code of the method svg where both filename and _filename are used. For consistency, the same principle is used in all methods.

Needs review!

comment:95 in reply to: ↑ 94 ; follow-up: Changed 6 months ago by vdelecroix

Replying to slabbe:

Replying to vdelecroix:

Why do you prefix filename_png, filename_pdf, etc with an additional underscore, ie _filename_png, _filename_pdf?

The reason is to distinguish the variable filename (input given from the user) from the variable _filename (name of a temporary file created by the method). See the code of the method svg where both filename and _filename are used. For consistency, the same principle is used in all methods.

Needs review!

Instead of the very unconventional _filename variable name you would better use temporary_filename, temporary_filename_tex, temporary_filename_pdf, etc.

comment:96 Changed 6 months ago by git

  • Commit changed from f1e4728d00efd130a0688ac79e616bffbae0c539 to 2f4771798e2c4219794d36890babbf65a44f2cfd

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

2f4771720343: rename _filename* -> temp_filename*

comment:97 in reply to: ↑ 95 Changed 6 months ago by slabbe

Replying to vdelecroix:

Instead of the very unconventional _filename variable name you would better use temporary_filename, temporary_filename_tex, temporary_filename_pdf, etc.

Ok, I renamed it as s/_filename/temp_filename/g.

Needs review!

comment:98 Changed 6 months ago by vdelecroix

Instead of from_graph, from_poset, ... wouldn't it make more sense to have Graph.tikz(), Poset.tikz(), etc? In the former design the TikzPicture code would end up with a ton of methods dealing with various specific objects.

comment:99 Changed 6 months ago by vdelecroix

Or alternatively: __tikz__() if you want to hide it.

comment:100 Changed 6 months ago by caruso

See comment:17 :-)

comment:101 follow-up: Changed 6 months ago by vdelecroix

I agree with Xavier ! The current branch proposes to make TikzPicture.from_XXX the default conversion function. If the design is wrong, better to change it now than in a hypothetical future.

comment:102 in reply to: ↑ 101 Changed 6 months ago by slabbe

Replying to vdelecroix:

I agree with Xavier ! The current branch proposes to make TikzPicture.from_XXX the default conversion function. If the design is wrong, better to change it now than in a hypothetical future.

See also comment:54 from mkoeppe who suggested the same thing.

Then, please read follow-up comment:58, and answer to my answer in comment:58.

comment:103 follow-ups: Changed 6 months ago by vdelecroix

Ideally, TikzPicture should inherit from LatexExpr so that returning a TikzPicture from a _latex_ method would not be a breaking change.

For the purpose of merging this ticket quickly, the simplest way out is to move the from_XXX as private standalone functions def _from_XXX and stating explicitely that these are just demo functions that will be removed in the future.

comment:104 in reply to: ↑ 103 Changed 6 months ago by slabbe

Replying to vdelecroix:

Ideally, TikzPicture should inherit from LatexExpr so that returning a TikzPicture from a _latex_ method would not be a breaking change.

Well, LatexExpr already inherits from the Python class str which makes it full of methods that, as a user, I don't want to see in the list of methods seen with tab-completion on an object of type TikzPicture.

For the purpose of merging this ticket quickly, the simplest way out is to move the from_XXX as private standalone functions def _from_XXX and stating explicitely that these are just demo functions that will be removed in the future.

Ok, let me think about that again, maybe this Thursday at Bordeaux Sage Thursdays, we can discuss this and converge to choices on how this is done for graphs (tkz-berge vs graphviz + dot2tex) and for polytopes (the actual tikz() method vs a new tikz() or __tikz__() method).

comment:105 in reply to: ↑ 103 ; follow-up: Changed 6 months ago by slabbe

Replying to vdelecroix:

For the purpose of merging this ticket quickly, the simplest way out is to move the from_XXX as private standalone functions def _from_XXX and stating explicitely that these are just demo functions that will be removed in the future.

There are four such methods right now:

    def from_dot_string(cls, dotdata, prog='dot'):
    def from_graph(cls, graph, merge_multiedges=True,
    def from_graph_with_pos(cls, graph, scale=1, merge_multiedges=True,
    def from_poset(cls, poset, **kwds):

As you suggest, I can change those taking a graph or a poset sage object into a private method:

    def from_dot_string(cls, dotdata, prog='dot'):
    def _from_graph(cls, graph, merge_multiedges=True,
    def _from_graph_with_pos(cls, graph, scale=1, merge_multiedges=True,
    def _from_poset(cls, poset, **kwds):

But, there is no sage object for a dot string. So, do I keep from_dot_string as it is or do you want me to make it private as well?

Last edited 6 months ago by slabbe (previous) (diff)

comment:106 in reply to: ↑ 105 ; follow-up: Changed 6 months ago by vdelecroix

Replying to slabbe:

Replying to vdelecroix:

For the purpose of merging this ticket quickly, the simplest way out is to move the from_XXX as private standalone functions def _from_XXX and stating explicitely that these are just demo functions that will be removed in the future.

There are four such methods right now:

    def from_dot_string(cls, dotdata, prog='dot'):
    def from_graph(cls, graph, merge_multiedges=True,
    def from_graph_with_pos(cls, graph, scale=1, merge_multiedges=True,
    def from_poset(cls, poset, **kwds):

As you suggest, I can change those taking a graph or a poset sage object into a private method:

    def from_dot_string(cls, dotdata, prog='dot'):
    def _from_graph(cls, graph, merge_multiedges=True,
    def _from_graph_with_pos(cls, graph, scale=1, merge_multiedges=True,
    def _from_poset(cls, poset, **kwds):

But, there is no sage object for a dot string. So, do I keep from_dot_string as it is or do you want me to make it private as well?

It should be fine to keep from_dot_string.

The only reason to make the three others private is to avoid that sage users rely on them. They will be changed without notice.

comment:107 Changed 5 months ago by git

  • Commit changed from 2f4771798e2c4219794d36890babbf65a44f2cfd to 793fbdeba581155a5f21c2a07ec81f1de6a5e396

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

793fbde20343: from_graph, from_poset -> _from_graph, _from_poset

comment:108 in reply to: ↑ 106 Changed 5 months ago by slabbe

Replying to vdelecroix:

It should be fine to keep from_dot_string.

The only reason to make the three others private is to avoid that sage users rely on them. They will be changed without notice.

Ok, I did that. Needs review.

comment:109 Changed 5 months ago by vdelecroix

It would be better not to advertize them in the module docstring

- If dot2tex Sage optional package and graphviz are installed, then the following
- one liner works allowing to create a nice tikzpicture from a graph with
- vertices and edges placed according to graphviz::
-
-    sage: t = TikzPicture._from_graph(g)  # optional: dot2tex # long time (3s)
-

And you can add a sentence explaining the transducer example that comes after.

comment:110 Changed 5 months ago by vdelecroix

I would also add a sentence somewhere in the module docstring about the fact that tikz is a latex package (called pgf-tikz by the way) and a reference to the page https://www.ctan.org/pkg/pgf. You sort of assume that everybody knows tikz which is not a reasonable assumption.

comment:111 Changed 5 months ago by git

  • Commit changed from 793fbdeba581155a5f21c2a07ec81f1de6a5e396 to 06c8fda7e1b115e1ef35a0ab605ebe24f719efc2

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

253905420343:fixing 2 doctests + wrong trailing colons
1ed6a0c20343: change default from pdf2svg to pdftocairo
6111d2a20343: provide log if an error occurs in method png
21fe0d320343: change default value of usepackage to None
a424ed520343: use shell=False instead
f9dcd5e20343: add doctest showing spaces in filename
ad234af20343: adding missing optional latex tag
32f5efa20343: using else instead of elif
dde118620343: rename _filename* -> temp_filename*
06c8fda20343: from_graph, from_poset -> _from_graph, _from_poset

comment:112 Changed 5 months ago by slabbe

I rebased the branch on 9.6.beta2. No new commit for now.

comment:113 Changed 5 months ago by git

  • Commit changed from 06c8fda7e1b115e1ef35a0ab605ebe24f719efc2 to 83b367cb93f1ba8a974884669939250f72d2d4f2

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

83b367cRemoved _from_graph from module doc

comment:114 Changed 5 months ago by vdelecroix

The following reference will not work :mod:`sage.misc.latex.py` (you need to remove the .py).

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

comment:115 Changed 5 months ago by git

  • Commit changed from 83b367cb93f1ba8a974884669939250f72d2d4f2 to bb314c215cd5797ff979e792882c0e5dcc5086a5

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

bb314c220343: adding info about pgf/tikz + removed acknowledgement since the code has evolved so much that it has nothing to do with sage.misc.latex anymore

comment:116 Changed 5 months ago by git

  • Commit changed from bb314c215cd5797ff979e792882c0e5dcc5086a5 to 55649f6924baab86e35045f9a1e8876c385e2fb9

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

55649f620343: removed a TODO

comment:117 Changed 5 months ago by slabbe

Needs review!

comment:118 Changed 5 months ago by vdelecroix

pycodestyle is broken as it does not detect the prefix r in front of the string...

src/sage/plot/tikzpicture.py:258:34: W605 invalid escape sequence '\d'
src/sage/plot/tikzpicture.py:259:34: W605 invalid escape sequence '\e'

comment:119 Changed 5 months ago by vdelecroix

  • Status changed from needs_review to needs_work

Your module does not seem to be included in the reference manual. You have to add an entry to src/doc/en/reference/misc/index.rst.

comment:120 Changed 5 months ago by git

  • Commit changed from 55649f6924baab86e35045f9a1e8876c385e2fb9 to e6d0a972bd3ec403f2ae1f48043072e5a1eca849

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

f8f49dc20343: fixing few """ to r"""
e6d0a9720343: adding module in doc + further doc improvements

comment:121 Changed 5 months ago by slabbe

  • Status changed from needs_work to needs_review

Needs review!

comment:122 follow-up: Changed 5 months ago by vdelecroix

I did not notice before but why did you put your module in sage/plot rather than sage/misc (where all the current latex code is)?

comment:123 in reply to: ↑ 122 ; follow-up: Changed 5 months ago by slabbe

Replying to vdelecroix:

I did not notice before but why did you put your module in sage/plot rather than sage/misc (where all the current latex code is)?

This was suggested in comment:34 by mkoeppe, to which I agree. The goal of the module is about drawing stuff, so it is natural to put it in sage/plot.

It is not true that all latex code is in misc. You also have graph_latex.py file in sage/graphs.

Also, to my opinion, sage/misc/latex.py is not soooo well-written (I repeat, this is an opinion). It is a module on which it is tough to build on top / reuse. You may want to look at the code which implements view to get an example. Who really wants to use this? I think the future will be more active if the features present in that module are re-thought and re-written else where and eventually it gets just replaced. The deprecation of the have_latex and friends methods done in #32650 is one of the many steps towards that. The creation of a module to deal with tikzpicture is another step towards that.

Last edited 5 months ago by slabbe (previous) (diff)

comment:124 in reply to: ↑ 123 Changed 5 months ago by mkoeppe

Replying to slabbe:

This was suggested in comment:34 by mkoeppe, to which I agree.

Not sure if I still agree with myself ;)

comment:125 Changed 5 months ago by vdelecroix

It might be true that sage.misc.latex is a mess. But given that

  1. tikzpicture.py shares some functionalities with sage.misc.latex (including code duplication)
  2. sage.misc.latex is widely used
  3. you claim that StandaloneTex/TikzPicture is part of the future of sage.misc.latex

I would like to

  1. have an idea of what is the future of sage.misc.latex (you said that a step forward but there is no destination)
  2. have the tools that will replace sage.misc.latex close to it in the sage source code or at the very least a mention of them in its documentation

What I want to avoid is that this ticket only adds a module n+1 to sagemath which does a small variation of module 1, module 2 and module n independently of all of them.

comment:126 Changed 5 months ago by slabbe

  • Status changed from needs_review to needs_work

Ok, let me do more changes (next week).

comment:127 Changed 5 months ago by git

  • Commit changed from e6d0a972bd3ec403f2ae1f48043072e5a1eca849 to ba46d5125b59988ca8690f55c62a45408374597f

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

92e844320343: mv plot/tikzpicture.py -> misc/latex_standalone.py
8a0903220343: mv plot/tikzpicture.py -> misc/latex_standalone.py (in documentation index.rst)
58e7acd20343: plot.tikzpicture -> misc.latex_standalone
d15ce2920343: StandaloneTex -> Standalone
ba46d5120343: improved module doc + enhance the Standalone module

comment:128 Changed 5 months ago by git

  • Commit changed from ba46d5125b59988ca8690f55c62a45408374597f to fb5c4f56ccdbb2e4631a73cfd046b6b1dff4e5f4

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

fb5c4f520343: fix pdftocairo feature import

comment:129 Changed 5 months ago by git

  • Commit changed from fb5c4f56ccdbb2e4631a73cfd046b6b1dff4e5f4 to d4f4f426e4eb82df1ea2f96305ee14248b3d136b

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

d4f4f4220343: improved class and module doc

comment:130 Changed 5 months ago by slabbe

  • Status changed from needs_work to needs_review

Gave more importance to the Standalone class. Updated and improved the documentation.

Needs review!

comment:131 Changed 5 months ago by caruso

  • Branch changed from u/slabbe/20343 to u/caruso/20343

comment:132 Changed 5 months ago by slabbe

  • Branch changed from u/caruso/20343 to u/slabbe/20343
  • Commit changed from d4f4f426e4eb82df1ea2f96305ee14248b3d136b to 4e4b4a4a89c405e245b77bcb405b5ec9ab713627

New commits:

9173d51typos
82ab15d20343: adding experimental decorator to from_graph methods and removing the underscore prefix
ddf2c3920343: using check call to open the viewer
fa55a0120343: adding stackoverflow url
4e4b4a420343: fixing a warning during docbuild

comment:133 Changed 5 months ago by slabbe

Needs review!

comment:134 Changed 5 months ago by mkoeppe

+Loading few latex packages::

-> "a" few

comment:135 Changed 5 months ago by mkoeppe

  • Cc chapoton added
+#*****************************************************************************
+#       Copyright (C) 2015-2022 Sébastien Labbé <slabqc@gmail.com>
+#
+#  Distributed under the terms of the GNU General Public License version 2 (GPLv2)
+#
+#  The full text of the GPLv2 is available at:
+#
+#                  http://www.gnu.org/licenses/

This is not our standard license header; are you sure you want to mark it as "v2 only"? Other files say "Distributed under the terms of the GNU General Public License (GPL); either version 2 of the License, or (at your option) any later version."

comment:136 Changed 5 months ago by mkoeppe

+Adding a border in the options avoids croping the vertices of a graph::

-> cropping

comment:137 follow-up: Changed 5 months ago by mkoeppe

+        sage: t = TikzPicture(s, standalone_config=["border=4mm"], usepackage=['tkz-graph']) # optional latex
+        sage: _ = t.pdf(view=False)             # long time (2s) # optional latex

This should also be marked # optional - latex_package_tkz_graph (occurs several times)

comment:138 follow-up: Changed 5 months ago by mkoeppe

+            sage: dotdata = G.graphviz_string()
+            sage: tikz = TikzPicture.from_dot_string(dotdata) # optional dot2tex # long time (3s)

this should probably also be marked as # optional - graphviz

comment:139 follow-ups: Changed 5 months ago by mkoeppe

Also, could you please mark the doctests using graphs as # optional - sage.graphs and the doctests using posets as # optional - sage.combinat?

comment:140 Changed 5 months ago by mkoeppe

Overall looking great!

comment:141 Changed 5 months ago by slabbe

Thanks for your review! I will improve the code accordingly.

comment:142 Changed 5 months ago by git

  • Commit changed from 4e4b4a4a89c405e245b77bcb405b5ec9ab713627 to 3ed5d51a47ae8c3dd3c8090f33e73422b63d393f

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

7cf9af520343: fixing licence and typos
3ed5d5120343: adding an example showing usetikzlibrary in module doc

comment:143 Changed 5 months ago by slabbe

It remains to fix the doctests tags. I will do this later.

comment:144 in reply to: ↑ 137 Changed 5 months ago by slabbe

Replying to mkoeppe:

+        sage: t = TikzPicture(s, standalone_config=["border=4mm"], usepackage=['tkz-graph']) # optional latex
+        sage: _ = t.pdf(view=False)             # long time (2s) # optional latex

This should also be marked # optional - latex_package_tkz_graph (occurs several times)

Only the line t.pdf() really needs the latex_package_tkz_graph optional tag. So I added this tag where necessary. Creating a Python string containing '\usetikzlibrary{tkz-graph}' by itself does not depend on the latex package. So, I did not added the optional tag everywhere.

comment:145 in reply to: ↑ 138 Changed 5 months ago by slabbe

Replying to mkoeppe:

+            sage: dotdata = G.graphviz_string()
+            sage: tikz = TikzPicture.from_dot_string(dotdata) # optional dot2tex # long time (3s)

this should probably also be marked as # optional - graphviz

At some point in time, it was changed that the installation of dot2tex fails if graphviz is not installed. But, whatever, maybe it is better to add graphviz optional tag as well, which I did (commits to be pushed soonafter).

comment:146 in reply to: ↑ 139 Changed 5 months ago by slabbe

Replying to mkoeppe:

Also, could you please mark the doctests using graphs as # optional - sage.graphs and the doctests using posets as # optional - sage.combinat?

Ok, done.

comment:147 Changed 5 months ago by git

  • Commit changed from 3ed5d51a47ae8c3dd3c8090f33e73422b63d393f to 36c3ebb5c2fa064b8c0f7e262ba2c3c06e9a9216

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

5f39c0620343: adding latex and latex_package_tkz_graph tags where necessary
8d981b620343:adding missing graphviz optional tag
36c3ebb20343: adding tag sage.graphs, sage.combinat, graphviz at some places

comment:148 in reply to: ↑ 139 Changed 5 months ago by slabbe

Replying to mkoeppe:

Also, could you please mark the doctests using graphs as # optional - sage.graphs and the doctests using posets as # optional - sage.combinat?

Also I could make the doctests less dependent on graphs stuff.

comment:149 Changed 5 months ago by git

  • Commit changed from 36c3ebb5c2fa064b8c0f7e262ba2c3c06e9a9216 to bc9186f91cb6a8f0d55d0e9eb1d7f18fe831dc1a

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

936e82d20343: updating example in docstrings of Standalone class to use Standalone class instead of inherited class TikzPicture
e90292c20343: adding methods to append to the lists of options
bc9186f20343: add a deprecation message for include_header -> content_only

comment:150 Changed 5 months ago by slabbe

Needs review!

comment:151 follow-up: Changed 5 months ago by mkoeppe

  • Reviewers changed from Jean-Philippe Labbé, Xavier Caruso, Vincent Delecroix to Jean-Philippe Labbé, Xavier Caruso, Vincent Delecroix, Matthias Koeppe

Looking great.

These lines and what depends on them should be marked # optional - sage.symbolic (for #32432, #32601)

+            sage: f(x) = -1 / x
+            sage: g(x) = 1 / (x + 1)

Other than that, from my side it's positive review.

comment:152 in reply to: ↑ 151 Changed 5 months ago by slelievre

Replying to mkoeppe:

These lines and what depends on them should be marked # optional - sage.symbolic (for #32432, #32601)

+            sage: f(x) = -1 / x
+            sage: g(x) = 1 / (x + 1)

Other than that, from my side it's positive review.

Alternatively, use lambda functions instead of symbolic ones:

-            sage: f(x) = -1 / x
-            sage: g(x) = 1 / (x + 1)
+            sage: f = lambda x: -1 / x
+            sage: g = lambda x: 1 / (x + 1)

comment:153 Changed 5 months ago by mkoeppe

No, they wouldn't show up well in the plots.

comment:154 follow-up: Changed 5 months ago by caruso

  • Status changed from needs_review to positive_review

Great! I think it is time to give a positive review to this ticket!

comment:155 Changed 5 months ago by git

  • Commit changed from bc9186f91cb6a8f0d55d0e9eb1d7f18fe831dc1a to 5c9b41c96e4213242fee4af98b934914fb4d2360
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

5c9b41c20343: adding doctest tag sage.symbolic

comment:156 Changed 5 months ago by slabbe

  • Summary changed from Adding sage/plot/tikzpicture.py to Adding sage/misc/latex_standalone.py + class TikzPicture

comment:157 in reply to: ↑ 154 Changed 5 months ago by slabbe

Replying to caruso:

Great! I think it is time to give a positive review to this ticket!

I added the doctests tags sage.symbolic to answer the latest comment. Needs review again. Sorry.

I also updated the title of the ticket.

comment:158 Changed 5 months ago by slabbe

  • Description modified (diff)

comment:159 Changed 5 months ago by slabbe

  • Description modified (diff)

comment:160 Changed 5 months ago by slabbe

Patchbot is green. We can change the status to positive review.

But, maybe before, we could check if all tests pass on a machine with no latex installed, because Volker is using such a computer to test tickets, see comment:85. Do someone has a machine with no latex? Or is there an easy way to mimic its absence?

comment:161 Changed 5 months ago by slelievre

  • Description modified (diff)
  • Summary changed from Adding sage/misc/latex_standalone.py + class TikzPicture to Add file `sage/misc/latex_standalone.py` and class `TikzPicture`

comment:162 Changed 5 months ago by slelievre

There must be a way to use tox for that...

comment:163 Changed 5 months ago by slabbe

I found a computer with no latex and no imagemagick:

$ pdflatex -v

La commande « pdflatex » n'a pas été trouvée, mais peut être installée avec :

sudo apt install texlive-latex-base
$ lualatex -v

La commande « lualatex » n'a pas été trouvée, mais peut être installée avec :

sudo apt install texlive-latex-base
$ convert -v

La commande « convert » n'a pas été trouvée, mais peut être installée avec :

sudo apt install graphicsmagick-imagemagick-compat  # version 1.4+really1.3.35-1, or
sudo apt install imagemagick-6.q16                  # version 8:6.9.10.23+dfsg-2.1ubuntu11.4
sudo apt install imagemagick-6.q16hdri              # version 8:6.9.10.23+dfsg-2.1ubuntu11.4

on which I installed sage:

$ ./sage -version
SageMath version 9.6.beta5, Release Date: 2022-03-12

on which I merged this ticket, I confirm the doctests work:

./sage -t src/sage/misc/latex_standalone.py 
no stored timings available
Running doctests with ID 2022-03-24-11-08-32-82df3068.
Git branch: 20343
Using --optional=debian,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,nauty,palp,pandoc,pdf2svg,pdftocairo,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file.
sage -t --random-seed=224621707555152601217192014946498015018 src/sage/misc/latex_standalone.py
    [216 tests, 0.29 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.3 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 0.3 seconds
Features detected for doctesting: graphviz,sage.combinat,sage.graphs,sage.symbolic
Pytest is not installed, skip checking tests that rely on it.
$ ./sage -t --optional=sage,optional,external --long src/sage/misc/latex_standalone.py 
too few successful tests, not using stored timings
Running doctests with ID 2022-03-24-11-08-54-06354475.
Git branch: 20343
Using --optional=debian,external,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,cplex,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,ffmpeg,graphviz,gurobi,imagemagick,internet,jupymake,kenzo,latex,latex_package_tkz_graph,latte_int,lrslib,lualatex,macaulay2,magma,maple,mathematica,matlab,mcqd,meataxe,nauty,octave,palp,pandoc,pdf2svg,pdflatex,pdftocairo,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scilab,sphinx,tdlib,xelatex
Doctesting 1 file.
sage -t --long --random-seed=318237908393715923968305484155979250546 src/sage/misc/latex_standalone.py
    [216 tests, 0.29 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.4 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 0.3 seconds
Features detected for doctesting: graphviz,pdftocairo,sage.combinat,sage.graphs,sage.symbolic
Pytest is not installed, skip checking tests that rely on it.

comment:164 Changed 5 months ago by caruso

  • Status changed from needs_review to positive_review

Nice! I give back a positive review to this ticket.

comment:165 Changed 5 months ago by slabbe

Super!

comment:166 Changed 4 months ago by vbraun

  • Branch changed from u/slabbe/20343 to 5c9b41c96e4213242fee4af98b934914fb4d2360
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:167 Changed 4 months ago by chapoton

  • Commit 5c9b41c96e4213242fee4af98b934914fb4d2360 deleted

this has introduced wrong indentation

src/sage/misc/latex_standalone.py:688:12: E111 indentation is not a multiple of four
src/sage/misc/latex_standalone.py:689:16: E111 indentation is not a multiple of four
src/sage/misc/latex_standalone.py:690:12: E111 indentation is not a multiple of four
src/sage/misc/latex_standalone.py:691:16: E111 indentation is not a multiple of four

comment:168 Changed 4 months ago by chapoton

so please swiftly review #33635

comment:169 follow-up: Changed 2 months ago by jhpalmieri

Could this possibly be leading to the bad LaTeX that's causing the problem with view(crystals.Tableaux("A3",shape=[2,1])) — see https://groups.google.com/g/sage-support/c/1u4m-Osz6jY/m/PAM-1C40AwAJ?

comment:170 Changed 2 months ago by jhpalmieri

In case this is relevant to the view(crystals....) failure, that is now #33947.

comment:171 in reply to: ↑ 169 Changed 2 months ago by slabbe

Replying to jhpalmieri:

Could this possibly be leading to the bad LaTeX that's causing the problem with view(crystals.Tableaux("A3",shape=[2,1])) — see https://groups.google.com/g/sage-support/c/1u4m-Osz6jY/m/PAM-1C40AwAJ?

No. The problem reported on sage-support is with respect to Sage 9.3. The TikzPicture code went into sage in 9.6. Also, for now, no other file is using the new TikzPicture module.

Note: See TracTickets for help on using tickets.