Opened 5 years ago
Closed 5 years ago
#22417 closed enhancement (fixed)
Add neighborly methods for polyhedra
Reported by: | moritz | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.6 |
Component: | geometry | Keywords: | polyhedron, days84 |
Cc: | jipilab, chapoton, vdelecroix | Merged in: | |
Authors: | Moritz Firsching | Reviewers: | Jean-Philippe 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 5 years ago by
- Branch set to u/moritz/neighborly
comment:2 Changed 5 years ago by
- Commit set to 941c9bdc05fada494102c2891948665a5d7a3750
comment:3 Changed 5 years ago by
Hi Moritz,
You could truncate the two long lines before the "for" (see https://www.python.org/dev/peps/pep-0008/#maximum-line-length )
comment:4 Changed 5 years ago by
and you can put your name as the author.
comment:5 Changed 5 years ago by
- Keywords days84 added
comment:6 Changed 5 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 5 years ago by
- Status changed from new to needs_review
comment:8 Changed 5 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 5 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 5 years ago by
- Commit changed from e147192732f9c9d2711e88b89fad10c1d9bbfd20 to 8552363654e92cd3a9d8afbaa26f6e68cf612cd8
comment:11 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:12 Changed 5 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 5 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 5 years ago by
- Description modified (diff)
- Keywords changed from polyhedron days84 to polyhedron, days84
- Reviewers set to Jean-Philippe Labbé
comment:15 Changed 5 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 5 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 5 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 5 years ago by
Thanks JP! I improved the docstring.
comment:19 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:20 Changed 5 years ago by
Looks good! Up to the bot, I'll give it a positive review.
comment:21 Changed 5 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 5 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 5 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 5 years ago by
thanks, JP!
I moved the text before the cubes to the cyclic example.
comment:25 Changed 5 years ago by
- Status changed from needs_review to positive_review
It now looks good to me. Positive review.
comment:26 Changed 5 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