Opened 5 years ago
Closed 5 years ago
#24047 closed defect (fixed)
Polyhedron.affine_hull() raises AssertionError
Reported by: | yzh | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.1 |
Component: | geometry | Keywords: | |
Cc: | mkoeppe, moritz, vdelecroix, jipilab | Merged in: | |
Authors: | Moritz Firsching | Reviewers: | Travis Scrimshaw, Jean-Philippe Labbé |
Report Upstream: | N/A | Work issues: | |
Branch: | 70114be (Commits, GitHub, GitLab) | Commit: | 70114bed83dee82e8d811ddb698022926fe33317 |
Dependencies: | Stopgaps: |
Description
The method affine_hull
of Polyhedron
fails.
sage: P1 = Polyhedron(vertices=([[-1, 1], [0, -1], [0, 0], [-1, -1]])) sage: P2 = Polyhedron(vertices=[[1, 1], [1, -1], [0, -1], [0, 0]]) sage: P = P1.intersection(P2) sage: A, b = P.affine_hull(as_affine_map=True, orthonormal=True, extend=True) AssertionError
This is because the order of the vertices of P is changed by the translation by zero vector.
sage: P.vertices() (A vertex at (0, 0), A vertex at (0, -1)) sage: P.translation(vector([0,0])).vertices() (A vertex at (0, -1), A vertex at (0, 0))
Change History (17)
comment:1 Changed 5 years ago by
- Cc moritz vdelecroix jipilab added
comment:2 follow-up: ↓ 3 Changed 5 years ago by
comment:3 in reply to: ↑ 2 Changed 5 years ago by
Replying to vdelecroix:
we should not recompute anything!
That would require #22701 - Setting up a Polyhedron from both Vrep and Hrep.
comment:4 Changed 5 years ago by
@Moritz: You told me you would have a fix for this? It consists in what exactly?
comment:5 Changed 5 years ago by
- Branch set to u/moritz/24047
comment:6 Changed 5 years ago by
- Commit set to 0022adca807b06c0eeb5786134d9531ebf005b42
- Status changed from new to needs_review
comment:7 Changed 5 years ago by
You have some tab characters (should be 4 spaces) and your indentation level is wrong:
Check that :trac:`24047` is fixed:: sage: P1 = Polyhedron(vertices=([[-1, 1], [0, -1], [0, 0], [-1, -1]])) sage: P2 = Polyhedron(vertices=[[1, 1], [1, -1], [0, -1], [0, 0]]) sage: P = P1.intersection(P2) sage: A, b = P.affine_hull(as_affine_map=True, orthonormal=True, extend=True)
comment:8 Changed 5 years ago by
- Status changed from needs_review to needs_work
comment:9 Changed 5 years ago by
- Commit changed from 0022adca807b06c0eeb5786134d9531ebf005b42 to 0bee595118118e9921038e59a987d99e90a4b3b1
Branch pushed to git repo; I updated commit sha1. New commits:
0bee595 | tabs to spaces
|
comment:10 Changed 5 years ago by
- Status changed from needs_work to needs_review
uups, I used a different editor, which changed spaces by tabs.. Should be fixed now.
comment:11 Changed 5 years ago by
comment:12 Changed 5 years ago by
- Status changed from needs_review to needs_work
that we really Q really
-> that Q really
?
Otherwise, it looks good to me and the bot is happy so I would put it as positive review once the small mistake above is corrected.
Thanks for the quick fix!
comment:13 Changed 5 years ago by
- Commit changed from 0bee595118118e9921038e59a987d99e90a4b3b1 to 70114bed83dee82e8d811ddb698022926fe33317
comment:14 Changed 5 years ago by
- Status changed from needs_work to positive_review
thanks for the review, JP!
comment:15 Changed 5 years ago by
reviewer name, please
comment:16 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw, Jean-Philippe Labbé
comment:17 Changed 5 years ago by
- Branch changed from u/moritz/24047 to 70114bed83dee82e8d811ddb698022926fe33317
- Resolution set to fixed
- Status changed from positive_review to closed
1) It would be bettter to sort the V and H generators so that this kind of troubles never appear (and equality test would be even faster)
2) The
translation
method is stupidly writtenwe should not recompute anything!