sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.31k stars 450 forks source link

Add a TikZ-tex output method for 2d and 3d polytopes #12083

Closed jplab closed 10 years ago

jplab commented 12 years ago

I recently had to draw polytopes for my work.

Many drawing tools already exist. But none of them satisfied me.

So far, I wrote a method that takes a polytope in 3d and a view-angle (taken from Jmol) and outputs a .tex file containing a TikZ image of the polytope.

The method should also be able to deal with 2-polytopes.

The result is highly customizable: colors, edge style, vertices style, etc...


First review: I made the suggested corrections. They are present in the patch trac12083_tikz_polytope_review.patch

Test passed on version 4.7.2

Apply attachment: trac12083_rebased_5.12.patch

Component: geometry

Keywords: TikZ, polytope

Author: Jean-Philippe Labbé

Reviewer: Sébastien Labbé, Volker Braun

Merged: sage-5.13.beta3

Issue created by migration from https://trac.sagemath.org/ticket/12083

novoselt commented 12 years ago
comment:1

Do you have a ready patch already? Such functionality can be quite awesome in conjunction with SageTeX!

jplab commented 12 years ago
comment:2

I just need to adapt the code a little bit. It is already mostly written and works with 'quite' big examples: A3,B3 and H3 permutahedra.

I just need to sit and do it: it should be there in a week or so...

jplab commented 12 years ago

Attachment: trac12083_tikz_polytope.patch.gz

Addition of tikz method for polyhedron

jplab commented 12 years ago
comment:3

Here is the patch. Please test it with your favorite 2d and 3d polytopes!

jplab commented 12 years ago

Attachment: trac12083_tikz_polytope.2.patch.gz

Addition of tikz method for polyhedron

seblabbe commented 12 years ago
comment:4
  1. Testing the file polyhedra.py creates junk in the working directory.
cd sage-4.8/devel/sage-slabbe/sage/geometry 
sage -t polyhedra.py
ls *.tex 
polytope-tikz.tex       polytope-tikz2.tex      polytope-tikz3.tex               

Saving files in your test is not necessary. I suggest you do the test like this instead :

sage: P1 = polytopes.small_rhombicuboctahedron()   
sage: t = P1.tikz([1,3,5], 175, scale=4)           
sage: type(t)                                      
<type 'str'>                                       
sage: print '\n'.join(t.splitlines()[:4])          
\begin{tikzpicture}%                               
        [x={(-0.939161cm, 0.244762cm)},            
        y={(0.097442cm, -0.482887cm)},             
        z={(0.329367cm, 0.840780cm)},                                          

If you want to keep those line in the doc (a good idea a believe), I suggest you mark them as untested :

sage: open('polytope-tikz2.tex', 'w').write(Image2)    # not tested
  1. You import n but you don't use it. (If you really needed it, I would have suggest to import the equivalent alias numerical_approx instead because the variable n is often used as an integer in the code...) But since you don't use it, just remove the import.
 from subprocess import Popen, PIPE                 
 from sage.misc.all import tmp_filename             
-from sage.misc.functional import norm              
+from sage.misc.functional import norm, n           
 from sage.misc.package import is_package_installed 
  1. Testing the file polyhedra.py is not significantly longer than before : Great!

sage-4.8:

$ sage -t polyhedra.py
sage -t  "devel/sage-slabbe/sage/geometry/polyhedra.py"
         [58.0 s]
----------------------------------------------------------------------                 
All tests passed!
Total time for all tests: 58.0 seconds

sage-4.8 + your patch:

$ sage -t polyhedra.py
sage -t  "devel/sage-slabbe/sage/geometry/polyhedra.py"
         [58.6 s]
----------------------------------------------------------------------                 
All tests passed!
Total time for all tests: 58.6 seconds
  1. You import VectorSpace but vector could do the job...
 from sage.modules.free_module_element import vector         
+from sage.modules.free_module import VectorSpace            
 from sage.matrix.constructor import matrix, identity_matrix 

In your code, you write :

V = VectorSpace(RDF,3) 
view_vector = V(view)  

I suggest you change this to :

view_vector = vector(RDF, view)
  1. Every methods must have doctests:
$ sage -coverage polyhedra.py
----------------------------------------------------------------------
polyhedra.py
SCORE polyhedra.py: 98% (214 of 217)

Missing doctests:
         * _tikz_2d(self, scale, edge_color, facet_color, opacity, vertex_color, axis):
         * _tikz_2d_in_3d(self, view, rot_angle, scale, edge_color, facet_color, opacity, vertex_color, axis):
         * _tikz_3d_in_3d(self, view, rot_angle, scale, edge_color, facet_color, opacity, vertex_color, axis):

----------------------------------------------------------------------
  1. I tested my script tikz2pdf on the above 3 junk tex files created by the doctest. Pdf files were created without problem. Pictures look great!

  2. Building the documentation creates 2 warnings :

$ sage -docbuild reference html
...
/Users/slabbe/Applications/sage-4.8/local/lib/python2.6/site-packages/sage/geometry/polyhedra.py:docstring
of sage.geometry.polyhedra.Polyhedron.tikz:26: WARNING: Inline literal
start-string without end-string.
/Users/slabbe/Applications/sage-4.8/local/lib/python2.6/site-packages/sage/geometry/polyhedra.py:docstring
of sage.geometry.polyhedra.Polyhedron.tikz:27: WARNING: Inline literal
start-string without end-string.
...

I believe the problem comes from this :

+            2) Select ``Console`;        
+            3) Select the tab ``State`;  
  1. Change this
            It read something like:

            moveto 0.0 {x y z angle} Scale   

to

            It read something like::

                moveto 0.0 {x y z angle} Scale   

It will look nicer in the documentation.

  1. I suggest to change the variable name rot_angle to angle.

  2. Input block do not follow the coding convention.

See www.sagemath.org/doc/developer/conventions.html

I suggest you follow this example :

    INPUT:

     - ``x`` - integer (default: 1) the description of the
       argument x goes here.  If it contains multiple lines, all
       the lines after the first need to be indented.

     - ``y`` - integer (default: 2) the ...

    OUTPUT:

    integer -- the ...

So, first say what is the type. Then, say what is the default (inside `` quotes. Then, give the description.

For the output, just say the type and what it is. The part ``tikz_pic`` is not necessary.

  1. The description of view argument could be improved.
- ``view`` -- a list of length 3 representing a vector; 

I know it is a vector. Is it the camera position? Is it the view angle? Is it the axis for the rotation?

  1. I think the type of what you return should be a LatexExpr instead of str.
from sage.misc.latex import LatexExpr

Compare latex(Image1) to latex(LatexExpr(Image1)). Because, sagetex will call latex function on your output...

Bonne fête !

jplab commented 12 years ago

Apply over the precedent patch

jplab commented 12 years ago

Description changed:

--- 
+++ 
@@ -7,3 +7,9 @@
 The method should also be able to deal with 2-polytopes.

 The result is highly customizable: colors, edge style, vertices style, etc...
+
+---
+
+First review: I made the suggested corrections. They are present in the patch trac12083_tikz_polytope_review.patch
+
+Test passed on version 4.7.2
jplab commented 12 years ago
comment:5

Attachment: trac12083_tikz_polytope_review.patch.gz

5d2aaf09-c963-473a-bf79-1f742a72700f commented 12 years ago

Attachment: trac_12083_unholy_rebase_to_sage-5.0.patch.gz

For illustrative purposes only

5d2aaf09-c963-473a-bf79-1f742a72700f commented 12 years ago
comment:6

I really like this functionality and I thought I'd help by rebasing the patch to sage-5.0, but I have now run out of time. Working on this reminded me of some things I don't like about the Projection class, but I am not sure about the best way to fix them. I think projections should be more accessible from Polyhedron objects, and vice-versa. Anyway, I did complete a patch for sage-5.0 that manages to pass the original tests, but its not very pretty. Its inefficient, and relies on a parent_polyhedron method for projections that might be objectionable. But since it works, it might be helpful in creating a better version.

If no one else does, I might have time to revisit this in a week or so.

novoselt commented 12 years ago
comment:7

So - which patches have to be applied? I suspect they'll need rebasing after all Volker's refactoring.

novoselt commented 12 years ago

Author: Jean-Philippe Labbé

novoselt commented 12 years ago

Reviewer: Sébastien Labbé

fchapoton commented 11 years ago
comment:8

let us try and see

apply trac12083_tikz_polytope.2.patch trac12083_tikz_polytope_review.patch

jplab commented 11 years ago
comment:9

So, today I used the reviewed patch to draw a polytope using the library decorations of tikz.

I realized that I was sloppy to implement the edges; they are drawn twice. This also makes the compilation in LaTeX slower.

I will write a patch for version 5.3 that does not have repetitions (and containing the improvements in the rebased version).

jplab commented 11 years ago

Rebased version of the tikz method

jplab commented 11 years ago
comment:10

Attachment: trac12083_rebased_5.4.1.patch.gz

So, here is the rebased version of the patch.

I solved a few problems: The vertices and edges appeared twice before (sloppy implementation); There was one vertex not drawn in the first (unholy) rebased version; The facets were badly drawn (problem with vertices naming in the code); The back edges were badly recognized.

I looked at the compiled tikz output examples and now everything seems in order.

All tests passed on 5.4.1

One thing I don't really like though is the fact that we need to pass to projections to access the tikz method. Perhaps a wrapper would do the thing?

jplab commented 11 years ago
comment:11

For the bot:

apply trac12083_rebased_5.4.1.patch

fchapoton commented 11 years ago
comment:12

There is a problem with the lines

            v1 = self.coords[index1]
            v2 = self.coords[index2]

which appear three times, but the variables v1 and v2 are never used.

fchapoton commented 11 years ago
comment:13

Moreover, three tests are failing on sage 5.8.beta1

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:14

This patch should probably be rebased. It would be nice to add in the ticket's description which patch are to be applied, too.

Nathann

fchapoton commented 11 years ago

Description changed:

--- 
+++ 
@@ -13,3 +13,5 @@
 First review: I made the suggested corrections. They are present in the patch trac12083_tikz_polytope_review.patch

 Test passed on version 4.7.2
+
+Apply [attachment: trac12083_rebased_5.12.patch](https://github.com/sagemath/sage-prod/files/10654100/trac12083_rebased_5.12.patch.gz)
fchapoton commented 11 years ago
comment:16

here is a rebased patch, passing tests and working on 5.12.beta0

apply only trac12083_rebased_5.12.patch​

fchapoton commented 11 years ago
comment:17

apply only trac12083_rebased_5.12.patch

vbraun commented 10 years ago

Changed reviewer from Sébastien Labbé to Sébastien Labbé, Volker Braun

vbraun commented 10 years ago
comment:18

lgtm

jdemeyer commented 10 years ago
comment:19

The PDF doc doesn't build:

! Package inputenc Error: Keyboard character used is undefined
(inputenc)                in inputencoding `utf8'.

See the inputenc package documentation for explanation.
Type  H <return>  for immediate help.
 ...

l.17893 \item[{^^Hegin\{tikzpicture\}\%}] 
                                          \leavevmode

To fix this, use r""" docstrings instead of """.

fchapoton commented 10 years ago

Attachment: trac12083_rebased_5.12.patch.gz

fchapoton commented 10 years ago
comment:21

I have made a corrected patch, hopefully.

apply trac12083_rebased_5.12.patch

vbraun commented 10 years ago
comment:22

Works for me...

jdemeyer commented 10 years ago

Merged: sage-5.13.beta3

kcrisman commented 10 years ago
comment:24

I just want to say that I could have used this a couple years ago and instead had to do lots of guess-and-check with graph plots, then convert to xfig... thank you, this will be very useful!