Opened 5 years ago
Closed 5 years ago
#22416 closed enhancement (fixed)
Add .is_inscribed() method to polytopes
Reported by:  jipilab  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  geometry  Keywords:  polytope, circumcenter, days84 
Cc:  moritz, mkoeppe  Merged in:  
Authors:  JeanPhilippe Labbé  Reviewers:  Frédéric Chapoton, Thierry Monteil 
Report Upstream:  N/A  Work issues:  
Branch:  d9e5415 (Commits, GitHub, GitLab)  Commit:  d9e54156cb0922438fd745ebb080c856036149ec 
Dependencies:  Stopgaps: 
Description (last modified by )
A fulldimensional compact polytope is inscribed if there exists a point in space which is equidistant to all its vertices.
This function checks if this point exists and returns it if asked for.
Change History (25)
comment:1 Changed 5 years ago by
 Description modified (diff)
 Summary changed from Add .is_circumscribable() method to polytopes to Add .is_inscribable() method to polytopes
comment:2 Changed 5 years ago by
 Branch set to u/jipilab/inscribable
comment:3 Changed 5 years ago by
 Commit set to ecaf3ee45f1a9923850012c469e0eea832350685
comment:4 Changed 5 years ago by
 Commit changed from ecaf3ee45f1a9923850012c469e0eea832350685 to 2a831bdc2f81ce3661616311ece22a520115c8e9
Branch pushed to git repo; I updated commit sha1. New commits:
2a831bd  Added a comment on the method used.

comment:5 Changed 5 years ago by
 Status changed from new to needs_review
comment:6 Changed 5 years ago by
 Commit changed from 2a831bdc2f81ce3661616311ece22a520115c8e9 to ef42b4a18fb9f9ceca45994da9287c3df083cc20
Branch pushed to git repo; I updated commit sha1. New commits:
ef42b4a  corrected indentation

comment:7 Changed 5 years ago by
 Commit changed from ef42b4a18fb9f9ceca45994da9287c3df083cc20 to d3dce249c5399b5af1e2e724337dd6b69f925d5b
Branch pushed to git repo; I updated commit sha1. New commits:
d3dce24  Added examples with NotImplemented

comment:8 Changed 5 years ago by
 Commit changed from d3dce249c5399b5af1e2e724337dd6b69f925d5b to 8e6df076125a573dd219a8240f4f622f7cc2a935
Branch pushed to git repo; I updated commit sha1. New commits:
8e6df07  Updated the next to python3 standard

comment:9 Changed 5 years ago by
 Keywords days84 added
comment:10 Changed 5 years ago by
 Commit changed from 8e6df076125a573dd219a8240f4f622f7cc2a935 to 21ec0e70509c26f57f7cc1997ef832c21e5a1e75
Branch pushed to git repo; I updated commit sha1. New commits:
21ec0e7  Changed keyword to certificate

comment:11 Changed 5 years ago by
Here are few first small comments after first reading:
 you forgot to replace
certify
withcertificate
in the code of the function.  inscribable is about the possibility to find a combinatorially equivalent polytope that can be inscribed in a sphere. So, i guess the current method should be called
is_inscribed
instead (if you agree, do not forget to also modify the ticket title).  i do not see any reason why the polytope should be fulldimensional. In case it is not, there might be various centers, but you could return the one that belongs to the affine hull of the polytope.
 you could be lazier by not constructing the list
other_vertices
, but an iterator instead (theall
call might end far before testing all points).  since you only do comparisons with the circumradius, you should not compute it, but use its square (computing the square root can be very costly in some rings).
row_data
shoud beraw_data
. Why not returning a boolean when
certificate
is set toFalse
? The problem here is thatbool((False,None))
isTrue
, so that this will cause issues if the method is used withinif
statements.
comment:12 Changed 5 years ago by
 Description modified (diff)
 Summary changed from Add .is_inscribable() method to polytopes to Add .is_inscribed() method to polytopes
comment:13 Changed 5 years ago by
 Commit changed from 21ec0e70509c26f57f7cc1997ef832c21e5a1e75 to 820910a6118298c3d24e4925c1a9a5b7adc62126
Branch pushed to git repo; I updated commit sha1. New commits:
820910a  First edits after review

comment:14 Changed 5 years ago by
 Commit changed from 820910a6118298c3d24e4925c1a9a5b7adc62126 to a563094ca0140e18bd79691d9adfdb4411ee5aa6
Branch pushed to git repo; I updated commit sha1. New commits:
a563094  Second small edits

comment:15 Changed 5 years ago by
 Commit changed from a563094ca0140e18bd79691d9adfdb4411ee5aa6 to 361914c844bc934c51487308aff077351cffab36
Branch pushed to git repo; I updated commit sha1. New commits:
361914c  Make tests pass

comment:16 Changed 5 years ago by
Some more comments:
 The documentation should start with a onesentence description, see http://doc.sagemath.org/html/en/developer/coding_basics.html#documentationstrings
 in describing the input, you should say that the default for
certificate
is False (look at other parts of Sage source code, as well as http://doc.sagemath.org/html/en/developer/coding_basics.html#documentationstrings)  the
OUTPUT
section should separate cases depending on the value ofcertificate
.  tests like
sage: q.is_inscribed(True)
could be misleading for the user. Where does theTrue
comes from. You can write this since certificate is the first option, but it is not a required parameter, so you should prefer the more explicitsage: q.is_inscribed(certificate=True)
. simplex_vertices
works because you assume that the polytope is fulldimensional, you should add a short comment to explain that just before that command, so that we understand what happens (and it will ease the work of someone who might try to extend the method to nonfulldimensional polytopes). since you do not use
other_vertices
anymore, you should remove its definition  you have:
if not self.is_compact() or not self.is_full_dimensional(): raise NotImplementedError("This function is implemented for fulldimensional polytopes only.")
If the polyhedron is fulldimensional but not compact, the user will get a misleading message. So make two different tests/statements, this adds informations to the user.
comment:17 Changed 5 years ago by
 Commit changed from 361914c844bc934c51487308aff077351cffab36 to 649a3b4af20708a24bdd91b854dc633f1e624ced
comment:18 Changed 5 years ago by
All test pass on 7.6.b4.
comment:19 followup: ↓ 20 Changed 5 years ago by
Two minor things. For the INPUT:
, you should format it as:
 ``certificate``  (default: ``False``) boolean; specifies whether to return the circumcenter, if found
Also, we don't technically consider error messages to be full sentences (at least the "shorter" ones like here) following Python's convention. So it should start with a lower case and not end with a period.
comment:20 in reply to: ↑ 19 Changed 5 years ago by
Thanks for the tip! Will do!
comment:21 Changed 5 years ago by
 Commit changed from 649a3b4af20708a24bdd91b854dc633f1e624ced to 4d9b5c50a9cdb5953507ec85f5b2f6c7e44ce646
Branch pushed to git repo; I updated commit sha1. New commits:
4d9b5c5  Added a indentation for doc to build

comment:22 Changed 5 years ago by
 Commit changed from 4d9b5c50a9cdb5953507ec85f5b2f6c7e44ce646 to d9e54156cb0922438fd745ebb080c856036149ec
Branch pushed to git repo; I updated commit sha1. New commits:
d9e5415  Correct an indentation in input block

comment:23 Changed 5 years ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be
comment:24 Changed 5 years ago by
 Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Thierry Monteil
comment:25 Changed 5 years ago by
 Branch changed from u/jipilab/inscribable to d9e54156cb0922438fd745ebb080c856036149ec
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
pep8 conventions