Opened 2 years ago
Closed 2 years ago
#29229 closed enhancement (fixed)
Improvements for `is_reflexive` for polyhedra over the integers
Reported by: | gh-kliem | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | geometry | Keywords: | polytopes, is_reflexive |
Cc: | jipilab, gh-LaisRast | Merged in: | |
Authors: | Jonathan Kliem | Reviewers: | Jean-Philippe Labbé |
Report Upstream: | N/A | Work issues: | |
Branch: | 27fa75c (Commits, GitHub, GitLab) | Commit: | 27fa75c21ba91bdae4204bb9c9e7f92b9cf75d1e |
Dependencies: | Stopgaps: |
Description (last modified by )
Currently, this method has almost no documentation.
Also it relies on polar
to work differently for Polyhedron_ZZ
than for Polyhedron_base
. However, this is not being tested at all. We fix this by checking being reflexive just from the inequalities.
This also improves the error messages. We raise a ValueError
if the polyhedron is unbounded. If the polytope does not have the origin in the interior, we return False
, as this is clearly not reflexive.
Change History (12)
comment:1 Changed 2 years ago by
- Branch set to public/29229
- Commit set to 1d7188951925228b0fbf9837542f153115b5cabd
- Keywords polytopes is_reflexive added
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
- Description modified (diff)
comment:3 follow-up: ↓ 4 Changed 2 years ago by
- Status changed from needs_review to needs_work
Some remarks:
- A lattice polytope is reflexive if it is polar to a lattice polytope. + A lattice polytope is reflexive if its polar is a lattice polytope.
Even though the raised error is not wrong as written (that it is not a polytope), I would still say "the polyhedron is not compact" or "the polyhedron should be compact". Simply because this avoids an eventual confusion in nomenclature polytope vs polyhedron.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 2 years ago by
Replying to jipilab:
Some remarks:
- A lattice polytope is reflexive if it is polar to a lattice polytope. + A lattice polytope is reflexive if its polar is a lattice polytope.
Those two statements are not equivalent. I would say that a polytope without origin in interior is not reflexive and that the return value shall be False
(and no error message). However, in this case the polar is not defined. This is why I chose this specific formulation.
Even though the raised error is not wrong as written (that it is not a polytope), I would still say "the polyhedron is not compact" or "the polyhedron should be compact". Simply because this avoids an eventual confusion in nomenclature polytope vs polyhedron.
comment:5 in reply to: ↑ 4 Changed 2 years ago by
Replying to gh-kliem:
Replying to jipilab:
Some remarks:
- A lattice polytope is reflexive if it is polar to a lattice polytope. + A lattice polytope is reflexive if its polar is a lattice polytope.Those two statements are not equivalent. I would say that a polytope without origin in interior is not reflexive and that the return value shall be
False
(and no error message). However, in this case the polar is not defined. This is why I chose this specific formulation.
Then, one should be transparent. Why not something like:
+ A lattice polytope is reflexive if it contains the origin and its polar with respect to the origin is a lattice polytope.
comment:6 Changed 2 years ago by
- Branch changed from public/29229 to public/29229-reb
- Commit changed from 1d7188951925228b0fbf9837542f153115b5cabd to 0718db85f5ca8f73f06d6854965b210c9a500259
- Status changed from needs_work to needs_review
comment:7 Changed 2 years ago by
- Commit changed from 0718db85f5ca8f73f06d6854965b210c9a500259 to c2abdd6f3b1f55b46dbc2002c54f6a2f9808c942
Branch pushed to git repo; I updated commit sha1. New commits:
c2abdd6 | typo in doc
|
comment:8 Changed 2 years ago by
- Reviewers set to Jean-Philippe Labbé
One last things:
- "the polyhedron should be compact" + "the polyhedron is not compact"
sounds better to me. A "should" gives it too many ways to be interpreted, whereas a "is not" is only reporting a fact, which I prefer. Not a big thing...
Patchbots look okay to me. So you can put this on positive review once you make this change on my behalf.
comment:9 Changed 2 years ago by
- Status changed from needs_review to needs_work
comment:10 Changed 2 years ago by
- Commit changed from c2abdd6f3b1f55b46dbc2002c54f6a2f9808c942 to 27fa75c21ba91bdae4204bb9c9e7f92b9cf75d1e
Branch pushed to git repo; I updated commit sha1. New commits:
27fa75c | improved error message
|
comment:11 Changed 2 years ago by
- Status changed from needs_work to positive_review
comment:12 Changed 2 years ago by
- Branch changed from public/29229-reb to 27fa75c21ba91bdae4204bb9c9e7f92b9cf75d1e
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
some improvements for `is_reflexive`