Opened 4 years ago
Closed 4 years ago
#22417 closed enhancement (fixed)
Add neighborly methods for polyhedra
Reported by:  moritz  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  geometry  Keywords:  polyhedron, days84 
Cc:  jipilab, chapoton, vdelecroix  Merged in:  
Authors:  Moritz Firsching  Reviewers:  JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  eea1215 (Commits, GitHub, GitLab)  Commit:  eea121525d892277a6706ff9297fafc527105c7b 
Dependencies:  Stopgaps: 
Description (last modified by )
I propose to add two functions to the polyhedron class; namely neighborliness
and is_neighborly
.
While neighborliness
could be used to calculate the is_neighborly
, it might be faster not to compute the neighborliness if one only wants to know is_neighborly(k)
for some small k. (More specifically, the output of P.is_neighborly()
shoud be the same as P.neighborliness()>=floor(P.dim()/2))
).
Change History (26)
comment:1 Changed 4 years ago by
 Branch set to u/moritz/neighborly
comment:2 Changed 4 years ago by
 Commit set to 941c9bdc05fada494102c2891948665a5d7a3750
comment:3 Changed 4 years ago by
Hi Moritz,
You could truncate the two long lines before the "for" (see https://www.python.org/dev/peps/pep0008/#maximumlinelength )
comment:4 Changed 4 years ago by
and you can put your name as the author.
comment:5 Changed 4 years ago by
 Keywords days84 added
comment:6 Changed 4 years ago by
 Commit changed from 941c9bdc05fada494102c2891948665a5d7a3750 to e147192732f9c9d2711e88b89fad10c1d9bbfd20
Branch pushed to git repo; I updated commit sha1. New commits:
e147192  improved docstring

comment:7 Changed 4 years ago by
 Status changed from new to needs_review
comment:8 Changed 4 years ago by
k = 1 while(True): if len(self.faces(k))==binomial(self.n_vertices(),k+1): k += 1 else: return k
can be simplified to
k = 1 while len(self.faces(k)) == binomial(self.n_vertices(), k + 1): k += 1 return k
(please be careful with spaces in the code)
comment:9 Changed 4 years ago by
 Status changed from needs_review to needs_work
About spacing, change (in the code and in the examples)
a=b
toa = b
,a>b
toa > b
, etcf(x,y)
tof(x, y)
comment:10 Changed 4 years ago by
 Commit changed from e147192732f9c9d2711e88b89fad10c1d9bbfd20 to 8552363654e92cd3a9d8afbaa26f6e68cf612cd8
comment:11 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 4 years ago by
Hi Moritz,
 perhaps in the "neighborliness" add a SEEALSO pointing to "is_neighborly".
 In the examples of neighborliness there are spaces missing in
P=Polyhedron
in several instances.  In the INPUT and OUTPUT of
is_neighborly
there are backticks missing around onek
.
More comments to come...
comment:13 Changed 4 years ago by
 Commit changed from 8552363654e92cd3a9d8afbaa26f6e68cf612cd8 to be71a2ebf34906ee95969d6df01541d844a05023
Branch pushed to git repo; I updated commit sha1. New commits:
be71a2e  added SEEALSO, links to wikipedia and improved docstrings

comment:14 Changed 4 years ago by
 Description modified (diff)
 Keywords changed from polyhedron days84 to polyhedron, days84
 Reviewers set to JeanPhilippe Labbé
comment:15 Changed 4 years ago by
 Cc chapoton vdelecroix added
This ticket looks ready to go to me.
I'd like to have a second opinion though to make sure!
comment:16 Changed 4 years ago by
 Status changed from needs_review to needs_work
Hi Moritz,
Here are a couple of things to correct:
 the syntax for the seealso block (in both functions) should be corrected (see here)
 The seealso block should be before the examples
 The first example in
is_neighborly
should be easy. Put the first example at the end.  perhaps change the index
l
so that it is not confused with the number1
.
The doc compiles on 7.6.beta6
comment:17 Changed 4 years ago by
 Commit changed from be71a2ebf34906ee95969d6df01541d844a05023 to c9943235016cf104e1948a2b51878d8d9345edd4
Branch pushed to git repo; I updated commit sha1. New commits:
c994323  improved docstring

comment:18 Changed 4 years ago by
Thanks JP! I improved the docstring.
comment:19 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 4 years ago by
Looks good! Up to the bot, I'll give it a positive review.
comment:21 Changed 4 years ago by
 Commit changed from c9943235016cf104e1948a2b51878d8d9345edd4 to 604b46b06f4142ac6c3d2260a4da41b873e6b409
Branch pushed to git repo; I updated commit sha1. New commits:
604b46b  fixed two pep8 errors

comment:22 Changed 4 years ago by
I would adapt the text before the neighborliness examples (specifying the cubes).
All test pass on 7.6.beta6 and the documentation looks good.
Once the bot gives the green light and you corrected the above, I'd give it a positive review.
comment:23 Changed 4 years ago by
 Commit changed from 604b46b06f4142ac6c3d2260a4da41b873e6b409 to eea121525d892277a6706ff9297fafc527105c7b
Branch pushed to git repo; I updated commit sha1. New commits:
eea1215  moved comment about cyclic polytopes

comment:24 Changed 4 years ago by
thanks, JP!
I moved the text before the cubes to the cyclic example.
comment:25 Changed 4 years ago by
 Status changed from needs_review to positive_review
It now looks good to me. Positive review.
comment:26 Changed 4 years ago by
 Branch changed from u/moritz/neighborly to eea121525d892277a6706ff9297fafc527105c7b
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
undo accidental unwanted modification of truncation method