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

affine_basis does not always work when used with orthogonal or orthonormal #29116

Closed
LaisRast opened this issue Jan 31, 2020 · 14 comments
Closed

Comments

@LaisRast
Copy link

When using affine_basis of Polyhedron with orthogonal or orthonormal we get the following errors.

sage: V =[
....:    [1, 0, -1, 0, 0],
....:    [1, 0, 0, -1, 0],
....:    [1, 0, 0, 0, -1],
....:    [1, 0, 0, +1, 0],
....:    [1, 0, 0, 0, +1],
....:    [1, +1, 0, 0, 0]
....:     ]
sage: P = Polyhedron(V)
sage: P.affine_hull()
A 4-dimensional polyhedron in ZZ^4 defined as the convex hull of 6 vertices
sage: P.affine_hull(orthogonal=True) 
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: V-representation data requires a list of length ambient_dim
sage: P.affine_hull(orthonormal=True, extend=True)
A 4-dimensional polyhedron in AA^4 defined as the convex hull of 5 vertices and 1 line

This happens because the method for computing an affine basis depends on the order of the vertices. We fix that by computing an affine basis of the polyhedron in the right way.

Depends on #29127

CC: @jplab @kliem

Component: geometry

Keywords: polytopes, affine_hull

Author: Jonathan Kliem

Branch/Commit: 7d21717

Reviewer: Laith Rastanawi

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

@LaisRast LaisRast added this to the sage-9.1 milestone Jan 31, 2020
@kliem
Copy link
Contributor

kliem commented Jan 31, 2020

Dependencies: #29127

@kliem
Copy link
Contributor

kliem commented Jan 31, 2020

New commits:

a8359e0implement `a_maximal_chain` for combinatorial polyhedron
6c4c1c0implement `an_affine_basis` for polytopes
3d87ccfmore description in the docs
1b78e87used an_affine_basis to simplify is_inscribed
5e547b3check sign of circumcenter using all vertices of simplex
231c16fapply change to 29125
4fb0f6bhandling small-dimensional cases
3f6013btemporary fix
df7ec00get correct affine basis for affine hull with orthogonal/orthonormal

@kliem
Copy link
Contributor

kliem commented Jan 31, 2020

Author: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Jan 31, 2020

Commit: df7ec00

@kliem
Copy link
Contributor

kliem commented Jan 31, 2020

Branch: public/29116

@kliem
Copy link
Contributor

kliem commented Feb 19, 2020

Changed commit from df7ec00 to 8b2709e

@kliem
Copy link
Contributor

kliem commented Feb 19, 2020

Changed branch from public/29116 to public/29116-reb

@kliem
Copy link
Contributor

kliem commented Feb 19, 2020

New commits:

61dd4a5implement `a_maximal_chain` for combinatorial polyhedron
a18011fimproved documentation
2e9f25fexposed a_maximal_chain to polyhedron_base
0315652implement `an_affine_basis` for polytopes
554556dmore description in the docs
aa30035used an_affine_basis to simplify is_inscribed
08b263eapply change to 29125
d2d2a8ahandling small-dimensional cases
ba48b17get correct affine basis for affine hull with orthogonal/orthonormal
8b2709etemporary fix

@LaisRast
Copy link
Author

comment:4

Since #29127 is on "positive review", I think this one also should be on "positive review".
It looks good to me.
(pycodestyle warnings are not sourced from her)

@LaisRast
Copy link
Author

Reviewer: Laith Rastanawi

@LaisRast
Copy link
Author

comment:5

I forgot to mention:

-            # We implicitely translate the first vertex of the affine basis to zero.
+            # We implicitly translate the first vertex of the affine basis to zero.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2020

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

7d21717typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2020

Changed commit from 8b2709e to 7d21717

@vbraun
Copy link
Member

vbraun commented Mar 1, 2020

Changed branch from public/29116-reb to 7d21717

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