Opened 4 years ago

Closed 4 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:

Status badges

Description (last modified by jipilab)

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 4 years ago by jipilab

  • Description modified (diff)
  • Summary changed from Add .is_circumscribable() method to polytopes to Add .is_inscribable() method to polytopes

comment:2 Changed 4 years ago by jipilab

  • Branch set to u/jipilab/inscribable

comment:3 Changed 4 years ago by git

  • Commit set to ecaf3ee45f1a9923850012c469e0eea832350685

Branch pushed to git repo; I updated commit sha1. New commits:

ecaf3eepep8 conventions

comment:4 Changed 4 years ago by git

  • Commit changed from ecaf3ee45f1a9923850012c469e0eea832350685 to 2a831bdc2f81ce3661616311ece22a520115c8e9

Branch pushed to git repo; I updated commit sha1. New commits:

2a831bdAdded a comment on the method used.

comment:5 Changed 4 years ago by jipilab

  • Authors set to Jean-Philippe Labbé
  • Status changed from new to needs_review

comment:6 Changed 4 years ago by git

  • Commit changed from 2a831bdc2f81ce3661616311ece22a520115c8e9 to ef42b4a18fb9f9ceca45994da9287c3df083cc20

Branch pushed to git repo; I updated commit sha1. New commits:

ef42b4acorrected indentation

comment:7 Changed 4 years ago by git

  • Commit changed from ef42b4a18fb9f9ceca45994da9287c3df083cc20 to d3dce249c5399b5af1e2e724337dd6b69f925d5b

Branch pushed to git repo; I updated commit sha1. New commits:

d3dce24Added examples with NotImplemented

comment:8 Changed 4 years ago by git

  • Commit changed from d3dce249c5399b5af1e2e724337dd6b69f925d5b to 8e6df076125a573dd219a8240f4f622f7cc2a935

Branch pushed to git repo; I updated commit sha1. New commits:

8e6df07Updated the next to python3 standard

comment:9 Changed 4 years ago by jipilab

  • Keywords days84 added

comment:10 Changed 4 years ago by git

  • Commit changed from 8e6df076125a573dd219a8240f4f622f7cc2a935 to 21ec0e70509c26f57f7cc1997ef832c21e5a1e75

Branch pushed to git repo; I updated commit sha1. New commits:

21ec0e7Changed keyword to certificate

comment:11 Changed 4 years ago by tmonteil

Here are few first small comments after first reading:

  • you forgot to replace certify with certificate 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 (the all 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 be raw_data.
  • Why not returning a boolean when certificate is set to False ? The problem here is that bool((False,None)) is True, so that this will cause issues if the method is used within if statements.
Last edited 4 years ago by tmonteil (previous) (diff)

comment:12 Changed 4 years ago by jipilab

  • Description modified (diff)
  • Summary changed from Add .is_inscribable() method to polytopes to Add .is_inscribed() method to polytopes

comment:13 Changed 4 years ago by git

  • Commit changed from 21ec0e70509c26f57f7cc1997ef832c21e5a1e75 to 820910a6118298c3d24e4925c1a9a5b7adc62126

Branch pushed to git repo; I updated commit sha1. New commits:

820910aFirst edits after review

comment:14 Changed 4 years ago by git

  • Commit changed from 820910a6118298c3d24e4925c1a9a5b7adc62126 to a563094ca0140e18bd79691d9adfdb4411ee5aa6

Branch pushed to git repo; I updated commit sha1. New commits:

a563094Second small edits

comment:15 Changed 4 years ago by git

  • Commit changed from a563094ca0140e18bd79691d9adfdb4411ee5aa6 to 361914c844bc934c51487308aff077351cffab36

Branch pushed to git repo; I updated commit sha1. New commits:

361914cMake tests pass

comment:16 Changed 4 years ago by tmonteil

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 of certificate.
  • tests like sage: q.is_inscribed(True) could be misleading for the user. Where does the True 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 explicit sage: 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 4 years ago by git

  • Commit changed from 361914c844bc934c51487308aff077351cffab36 to 649a3b4af20708a24bdd91b854dc633f1e624ced

Branch pushed to git repo; I updated commit sha1. New commits:

c886ddbSecond edits after review
649a3b4pep8 and pyflakes conventions

comment:18 Changed 4 years ago by jipilab

All test pass on 7.6.b4.

comment:19 follow-up: Changed 4 years ago by tscrim

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 4 years ago by jipilab

Thanks for the tip! Will do!

comment:21 Changed 4 years ago by git

  • Commit changed from 649a3b4af20708a24bdd91b854dc633f1e624ced to 4d9b5c50a9cdb5953507ec85f5b2f6c7e44ce646

Branch pushed to git repo; I updated commit sha1. New commits:

4d9b5c5Added a indentation for doc to build

comment:22 Changed 4 years ago by git

  • Commit changed from 4d9b5c50a9cdb5953507ec85f5b2f6c7e44ce646 to d9e54156cb0922438fd745ebb080c856036149ec

Branch pushed to git repo; I updated commit sha1. New commits:

d9e5415Correct an indentation in input block

comment:23 Changed 4 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be

comment:24 Changed 4 years ago by tscrim

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Thierry Monteil

comment:25 Changed 4 years ago by vbraun

  • Branch changed from u/jipilab/inscribable to d9e54156cb0922438fd745ebb080c856036149ec
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.