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

Speed up incidence matrix of polyhedra #28643

Closed
kliem opened this issue Oct 22, 2019 · 16 comments
Closed

Speed up incidence matrix of polyhedra #28643

kliem opened this issue Oct 22, 2019 · 16 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 22, 2019

We speed up the calculation of the incidence matrix by avoiding recomputation.

The obvious way to implement it, is by doing a nested loop (one loop for Vrepresentation, one for Hrepresentation).
We do not change this.
However, the inner loop should not recompute things, e.g.

  • the vector of a representation element,
  • the index of a representation element,
  • how to compute the product of two representation element (this consists of nested methods again),
    instead call the product of two vectors directly.

Before:

sage: P = polytopes.permutahedron(7)
sage: %time _ = P.incidence_matrix()
CPU times: user 2.21 s, sys: 52.4 ms, total: 2.26 s
Wall time: 2.21 s
sage: P = polytopes.associahedron(['A',10],backend='normaliz')
sage: %time _ = P.incidence_matrix()
CPU times: user 21.7 s, sys: 129 ms, total: 21.9 s
Wall time: 21.7 s

After:

sage: P = polytopes.permutahedron(7)
sage: %time _ = P.incidence_matrix()
CPU times: user 682 ms, sys: 81.4 ms, total: 764 ms
Wall time: 606 ms
sage: P = polytopes.associahedron(['A',10],backend='normaliz')
sage: %time _ = P.incidence_matrix()
CPU times: user 6.16 s, sys: 67.5 ms, total: 6.23 s
Wall time: 6.16 s

CC: @jplab @LaisRast

Component: geometry

Keywords: polytopes, incidence matrix

Author: Jonathan Kliem

Branch/Commit: 6efb1bc

Reviewer: Laith Rastanawi

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

@kliem kliem added this to the sage-9.0 milestone Oct 22, 2019
@kliem
Copy link
Contributor Author

kliem commented Oct 22, 2019

Commit: 0e419eb

@kliem
Copy link
Contributor Author

kliem commented Oct 22, 2019

New commits:

0e419eboptimized incidence matrix

@kliem
Copy link
Contributor Author

kliem commented Oct 22, 2019

Branch: public/28643

@kliem

This comment has been minimized.

@LaisRast
Copy link

comment:3

It is faster and still correct.
One suggestion: I think it would be a good idea to add more examples in the docstring.
For instance, you can add an example for unbounded polyhedron:

sage: P = Polyhedron(vertices=[(1,0,0), (1,1,0)], rays=[(1,0,0)], lines=[(0,0,1)])
sage: P.incidence_matrix()
[1 1 1]
[1 1 0]
[1 0 1]
[0 1 1]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 22, 2019

Changed commit from 0e419eb to e50590c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 22, 2019

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

e50590cmore examples to incidence matrix docstring

@LaisRast
Copy link

comment:5

You wrote that "The incidence matrix is not unique up to permutation for unbounded polyhedra", I believe you meant something else.

Plus, it would be nice to include the following example (which you suggested before). This is an example of two "combinatorially isomorphic" polyhedra with different incidence matrices.

sage: P1 = Polyhedron(vertices=[[1,0],[-1,0]],rays=[[0,1]])
sage: P1.incidence_matrix()
[1 1 0]
[1 0 1]
[0 1 1]
sage: P2 = Polyhedron(vertices=[[1,0],[-1,0]],rays=[[0,1], [1,1]])
sage: P2.incidence_matrix()
[1 1 0]
[1 0 0]
[0 1 1]
[0 0 1]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 22, 2019

Changed commit from e50590c to a1738c8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 22, 2019

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

a1738c8improved examples

@LaisRast
Copy link

Reviewer: Laith Rastanawi

@LaisRast
Copy link

comment:7

I think it is good to go.

@jplab
Copy link

jplab commented Oct 24, 2019

comment:8

May I suggest:

-        The incidence matrix does not uniquely determine
-        an unbounded polyhedra::
+        An incidence matrix does not determine a unique 
+        polyhedra::

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2019

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

6efb1bcimproved docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2019

Changed commit from a1738c8 to 6efb1bc

@vbraun
Copy link
Member

vbraun commented Oct 27, 2019

Changed branch from public/28643 to 6efb1bc

@vbraun vbraun closed this as completed in 9627de6 Oct 27, 2019
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

4 participants