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: | sage-7.6 |
Component: | geometry | Keywords: | polytope, circumcenter, days84 |
Cc: | moritz, mkoeppe | Merged in: | |
Authors: | Jean-Philippe 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 full-dimensional 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 full-dimensional. 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 one-sentence description, see http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings
- 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#documentation-strings) - 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 full-dimensional, 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 non-full-dimensional 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 full-dimensional polytopes only.")
If the polyhedron is full-dimensional 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 follow-up: ↓ 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