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

Dilation of polyhedron with both Vrep and Hrep (if backend supports it) #29200

Closed
kliem opened this issue Feb 14, 2020 · 11 comments
Closed

Dilation of polyhedron with both Vrep and Hrep (if backend supports it) #29200

kliem opened this issue Feb 14, 2020 · 11 comments

Comments

@kliem
Copy link
Contributor

kliem commented Feb 14, 2020

Currently the dilation of a polyhedron is done by computing the new double description from the new vertices.

With #28880 at hand, we can specify both Vrep and Hrep and the backend will use both (if available) or the shorter description. Either way, this will improve performance of polyhedra with many vertices but few facets (e.g. a hypercube).

Before this ticket we had the following timings:

sage: P = polytopes.hypercube(14)
sage: %time _ = 2*P
CPU times: user 2.77 s, sys: 19.8 ms, total: 2.79 s
Wall time: 2.79 s
sage: P = polytopes.hypercube(7, backend='field')
sage: %time _ = 2*P
CPU times: user 3.58 s, sys: 35 µs, total: 3.58 s
Wall time: 3.58 s
sage: P = polytopes.cross_polytope(8, backend='field')
sage: %time _ = 2*P
CPU times: user 15.5 s, sys: 23.9 ms, total: 15.6 s
Wall time: 15.6 s
sage: P = polytopes.cross_polytope(13)
sage: %time _ = 2*P
CPU times: user 395 ms, sys: 0 ns, total: 395 ms
Wall time: 394 ms

With this ticket we have:

sage: P = polytopes.hypercube(14)
sage: %time _ = 2*P
CPU times: user 589 ms, sys: 6 µs, total: 589 ms
Wall time: 588 ms
sage: P = polytopes.hypercube(7, backend='field')
sage: %time _ = 2*P
CPU times: user 14.1 ms, sys: 0 ns, total: 14.1 ms
Wall time: 14.1 ms
sage: P = polytopes.cross_polytope(8, backend='field')
sage: %time _ = 2*P
CPU times: user 49.2 ms, sys: 0 ns, total: 49.2 ms
Wall time: 48.3 ms
sage: P = polytopes.cross_polytope(13)
sage: %time _ = 2*P
CPU times: user 428 ms, sys: 8.08 ms, total: 436 ms
Wall time: 436 ms

Note that for the last timing, the choice of only specifying Vrep was already good (at least for backend ppl). So with the new setup, thinks take a bit more time, as dilation computes the new Hrep, which ppl discards then.

CC: @jplab @LaisRast

Component: geometry

Keywords: polyhedra, dilation, precomputed data

Author: Jonathan Kliem

Branch: ea5eb69

Reviewer: Sébastien Labbé

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

@kliem kliem added this to the sage-9.1 milestone Feb 14, 2020
@kliem
Copy link
Contributor Author

kliem commented Feb 15, 2020

New commits:

ea5eb69use both Vrep and Hrep to obtain dilation

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Feb 15, 2020

Branch: public/29200

@kliem
Copy link
Contributor Author

kliem commented Feb 15, 2020

Author: Jonathan Kliem

@kliem
Copy link
Contributor Author

kliem commented Feb 15, 2020

Commit: ea5eb69

@kliem
Copy link
Contributor Author

kliem commented Feb 15, 2020

Changed keywords from none to polyhedra, dilation, precomputed data

@seblabbe
Copy link
Contributor

seblabbe commented Mar 5, 2020

Reviewer: Sébastien Labbé

@vbraun
Copy link
Member

vbraun commented Mar 8, 2020

Changed branch from public/29200 to ea5eb69

@seblabbe
Copy link
Contributor

seblabbe commented Mar 8, 2020

Changed commit from ea5eb69 to none

@seblabbe
Copy link
Contributor

seblabbe commented Mar 8, 2020

comment:4

We forgot to update one doctest, see #29300

@jplab
Copy link

jplab commented Mar 8, 2020

comment:5

Replying to @seblabbe:

We forgot to update one doctest, see #29300

Yep, for such changes, it is important to test with and without optional packages... otherwise, we tend to forget this. Good catch.

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