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

Small improvements for ppl backend #32157

Closed
kliem opened this issue Jul 7, 2021 · 14 comments
Closed

Small improvements for ppl backend #32157

kliem opened this issue Jul 7, 2021 · 14 comments

Comments

@kliem
Copy link
Contributor

kliem commented Jul 7, 2021

We remove some code duplications in the ppl backend. Along the way we also allow initialization from ppl_polyhedron. This saves almost all the time, when extending base ring to QQ:

Before:

sage: %time P = polytopes.permutahedron(7)                                                                                                                                                                                                                                                                                                                                 
CPU times: user 1.51 s, sys: 19.8 ms, total: 1.53 s
Wall time: 1.52 s
sage: %time P.base_extend(QQ)                                                                                                                                                                                                                                                                                                                                              
CPU times: user 1.17 s, sys: 7.78 ms, total: 1.18 s
Wall time: 1.18 s
A 6-dimensional polyhedron in QQ^7 defined as the convex hull of 5040 vertices

After:

sage: %time P = polytopes.permutahedron(7)                                                                                                                                                                                                                                                                                                                                 
CPU times: user 1.5 s, sys: 23.4 ms, total: 1.52 s
Wall time: 1.52 s
sage: %time P.base_extend(QQ)                                                                                                                                                                                                                                                                                                                                              
CPU times: user 177 ms, sys: 50 µs, total: 177 ms
Wall time: 176 ms
A 6-dimensional polyhedron in QQ^7 defined as the convex hull of 5040 vertices

CC: @jplab @mkoeppe @yuan-zhou

Component: geometry

Author: Jonathan Kliem

Branch/Commit: c82c144

Reviewer: Matthias Koeppe

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

@kliem kliem added this to the sage-9.4 milestone Jul 7, 2021
@kliem
Copy link
Contributor Author

kliem commented Jul 7, 2021

comment:2

This is a preliminary for two things:

  • Allowing lazy computation of Hrepresentation and Vrepresentation
  • Have interactive backends (add generator/add constraint).

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 8, 2021

comment:3

Why is the _element_constructor_polyhedron only needed for Polyhedra_QQ_ppl but not Polyhedra_ZZ_ppl?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

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

5124844add _element_constructore_polyhedron for ppl over ZZ

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

Changed commit from 5e81afa to 5124844

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 8, 2021

comment:5
+
+        See :class:`Polyhedron_normaliz` for a description of the input
+        data.
+

should be ...ppl

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 8, 2021

comment:6

We forgot to do this in the normaliz and polymake backends, but now that mutability is coming into play I think it's crucial to clarify object ownership: Does the normaliz cone / polymake polytope / ppl polyhedron belong to the created Polyhedron object?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

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

c82c144remove copy paste typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

Changed commit from 5124844 to c82c144

@kliem
Copy link
Contributor Author

kliem commented Jul 8, 2021

comment:8

Replying to @mkoeppe:

We forgot to do this in the normaliz and polymake backends, but now that mutability is coming into play I think it's crucial to clarify object ownership: Does the normaliz cone / polymake polytope / ppl polyhedron belong to the created Polyhedron object?

Yes, I think so. I don't know what exact consequences this has. Do we make a copy, when initializing with a cone etc?

As long as everything is immutable, it's fine to have multiple polyhedra with the same cone. However, once things are mutable, that should be taken care of.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 8, 2021

comment:9

Replying to @kliem:

Replying to @mkoeppe:

We forgot to do this in the normaliz and polymake backends, but now that mutability is coming into play I think it's crucial to clarify object ownership: Does the normaliz cone / polymake polytope / ppl polyhedron belong to the created Polyhedron object?

Yes, I think so. I don't know what exact consequences this has. Do we make a copy, when initializing with a cone etc?

If it is clarified that ownership transfers to the new object, then no copy needs to be made.

@kliem
Copy link
Contributor Author

kliem commented Jul 8, 2021

comment:10

Yes this makes sense. When giving a cone to a normaliz polyhedron, I would usually assume that it may be modified. If the polyhedron is mutable, this mean actual modification and not just computing stuff.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 8, 2021

comment:11

Right, and on the other hand it would be an error if the user continues to modify the object that now belongs to the polyhedron.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 17, 2021

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Jul 24, 2021

Changed branch from u/gh-kliem/small_improvements_ppl to c82c144

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

3 participants