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

Polyhedron_base.vertex_adjacency_matrix: Do not use face_lattice #32666

Closed
mkoeppe opened this issue Oct 11, 2021 · 14 comments
Closed

Polyhedron_base.vertex_adjacency_matrix: Do not use face_lattice #32666

mkoeppe opened this issue Oct 11, 2021 · 14 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 11, 2021

instead just go through self.combinatorial_polyhedron().edges()

This should be faster - and only avoids an indirect dependency of sage.geometry.polyhedron.plot on sage.combinat

CC: @kliem

Component: geometry

Author: Jonathan Kliem

Branch/Commit: 2cd253c

Reviewer: Frédéric Chapoton, Matthias Koeppe

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 11, 2021
@kliem
Copy link
Contributor

kliem commented Oct 11, 2021

New commits:

7db923ecompute vertex adjacency matrix from edges
21a6c94remove code duplications

@kliem
Copy link
Contributor

kliem commented Oct 11, 2021

Author: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Oct 11, 2021

Commit: 21a6c94

@kliem
Copy link
Contributor

kliem commented Oct 11, 2021

Branch: public/32666

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:2

ok, looks good

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 11, 2021

comment:3

Thanks for working on this!

@kliem
Copy link
Contributor

kliem commented Oct 11, 2021

comment:4

Thanks for the review.
This change turned out to be surprisingly uncomplicated.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 11, 2021

comment:5

how about

--- a/src/sage/geometry/polyhedron/combinatorial_polyhedron/base.pyx
+++ b/src/sage/geometry/polyhedron/combinatorial_polyhedron/base.pyx
@@ -1276,7 +1276,7 @@ cdef class CombinatorialPolyhedron(SageObject):
             sage: polytopes.cube().vertex_adjacency_matrix().is_immutable()
             True
         """
-        from sage.rings.all import ZZ
+        from sage.rings.integer_ring import ZZ
         from sage.matrix.constructor import matrix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 11, 2021

Changed commit from 21a6c94 to 2cd253c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 11, 2021

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2cd253cmore specific import

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 11, 2021

Changed reviewer from Frédéric Chapoton to Frédéric Chapoton, Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 11, 2021

comment:7

Thanks!

@vbraun
Copy link
Member

vbraun commented Oct 19, 2021

Changed branch from public/32666 to 2cd253c

mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
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