Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jplab opened this issue Nov 25, 2011 · 33 comments
Closed

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

jplab opened this issue Nov 25, 2011 · 33 comments

Comments

@jplab
Copy link

jplab commented Nov 25, 2011

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
Copy link
Member

comment:1

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

@jplab
Copy link
Author

jplab commented Nov 25, 2011

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
Copy link
Author

jplab commented Dec 4, 2011

Attachment: trac12083_tikz_polytope.patch.gz

Addition of tikz method for polyhedron

@jplab
Copy link
Author

jplab commented Dec 4, 2011

comment:3

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

@jplab
Copy link
Author

jplab commented Dec 4, 2011

Attachment: trac12083_tikz_polytope.2.patch.gz

Addition of tikz method for polyhedron

@seblabbe
Copy link
Contributor

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
Copy link
Author

jplab commented May 31, 2012

Apply over the precedent patch

@jplab

This comment has been minimized.

@jplab
Copy link
Author

jplab commented May 31, 2012

comment:5

Attachment: trac12083_tikz_polytope_review.patch.gz

@sagetrac-mhampton
Copy link
Mannequin

sagetrac-mhampton mannequin commented May 31, 2012

Attachment: trac_12083_unholy_rebase_to_sage-5.0.patch.gz

For illustrative purposes only

@sagetrac-mhampton
Copy link
Mannequin

sagetrac-mhampton mannequin commented May 31, 2012

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
Copy link
Member

comment:7

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

@novoselt
Copy link
Member

Author: Jean-Philippe Labbé

@novoselt
Copy link
Member

Reviewer: Sébastien Labbé

@fchapoton
Copy link
Contributor

comment:8

let us try and see

apply trac12083_tikz_polytope.2.patch trac12083_tikz_polytope_review.patch

@jplab
Copy link
Author

jplab commented Nov 26, 2012

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
Copy link
Author

jplab commented Dec 19, 2012

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
Copy link
Author

jplab commented Dec 19, 2012

comment:11

For the bot:

apply trac12083_rebased_5.4.1.patch

@fchapoton
Copy link
Contributor

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
Copy link
Contributor

comment:13

Moreover, three tests are failing on sage 5.8.beta1

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 20, 2013

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

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor

comment:16

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

apply only trac12083_rebased_5.12.patch​

@fchapoton
Copy link
Contributor

comment:17

apply only trac12083_rebased_5.12.patch

@vbraun
Copy link
Member

vbraun commented Nov 1, 2013

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

@vbraun
Copy link
Member

vbraun commented Nov 1, 2013

comment:18

lgtm

@jdemeyer
Copy link

jdemeyer commented Nov 1, 2013

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
Copy link
Contributor

Attachment: trac12083_rebased_5.12.patch.gz

@fchapoton
Copy link
Contributor

comment:21

I have made a corrected patch, hopefully.

apply trac12083_rebased_5.12.patch

@vbraun
Copy link
Member

vbraun commented Nov 4, 2013

comment:22

Works for me...

@jdemeyer
Copy link

jdemeyer commented Nov 9, 2013

Merged: sage-5.13.beta3

@kcrisman
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants