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

Status badges

Description (last modified by jipilab)

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 moritz

  • Branch set to u/moritz/neighborly

comment:2 Changed 4 years ago by git

  • Commit set to 941c9bdc05fada494102c2891948665a5d7a3750

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

941c9bdundo accidental unwanted modification of truncation method

comment:3 Changed 4 years ago by jipilab

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

and you can put your name as the author.

comment:5 Changed 4 years ago by moritz

  • Authors set to Moritz Firsching
  • Keywords days84 added

comment:6 Changed 4 years ago by git

  • Commit changed from 941c9bdc05fada494102c2891948665a5d7a3750 to e147192732f9c9d2711e88b89fad10c1d9bbfd20

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

e147192improved docstring

comment:7 Changed 4 years ago by moritz

  • Status changed from new to needs_review

comment:8 Changed 4 years ago by vdelecroix

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 vdelecroix

  • Status changed from needs_review to needs_work

About spacing, change (in the code and in the examples)

  • a=b to a = b, a>b to a > b, etc
  • f(x,y) to f(x, y)

comment:10 Changed 4 years ago by git

  • Commit changed from e147192732f9c9d2711e88b89fad10c1d9bbfd20 to 8552363654e92cd3a9d8afbaa26f6e68cf612cd8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

17e931eadded 'neighborliness' and 'is_neighborly' methods
a05912eundo accidental unwanted modification of truncation method
07e8c58improved docstring
8552363spaces and more doctests

comment:11 Changed 4 years ago by moritz

  • Status changed from needs_work to needs_review

Thanks Vincent! I modified the spaces and added more doctests.


New commits:

17e931eadded 'neighborliness' and 'is_neighborly' methods
a05912eundo accidental unwanted modification of truncation method
07e8c58improved docstring
8552363spaces and more doctests

comment:12 Changed 4 years ago by jipilab

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 one k.

More comments to come...

comment:13 Changed 4 years ago by git

  • Commit changed from 8552363654e92cd3a9d8afbaa26f6e68cf612cd8 to be71a2ebf34906ee95969d6df01541d844a05023

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

be71a2eadded SEEALSO, links to wikipedia and improved docstrings

comment:14 Changed 4 years ago by jipilab

  • Description modified (diff)
  • Keywords changed from polyhedron days84 to polyhedron, days84
  • Reviewers set to Jean-Philippe Labbé

comment:15 Changed 4 years ago by jipilab

  • 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 jipilab

  • 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 indexl so that it is not confused with the number 1.

The doc compiles on 7.6.beta6

comment:17 Changed 4 years ago by git

  • Commit changed from be71a2ebf34906ee95969d6df01541d844a05023 to c9943235016cf104e1948a2b51878d8d9345edd4

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

c994323improved docstring

comment:18 Changed 4 years ago by moritz

Thanks JP! I improved the docstring.

comment:19 Changed 4 years ago by moritz

  • Status changed from needs_work to needs_review

comment:20 Changed 4 years ago by jipilab

Looks good! Up to the bot, I'll give it a positive review.

comment:21 Changed 4 years ago by git

  • Commit changed from c9943235016cf104e1948a2b51878d8d9345edd4 to 604b46b06f4142ac6c3d2260a4da41b873e6b409

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

604b46bfixed two pep8 errors

comment:22 Changed 4 years ago by jipilab

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 git

  • Commit changed from 604b46b06f4142ac6c3d2260a4da41b873e6b409 to eea121525d892277a6706ff9297fafc527105c7b

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

eea1215moved comment about cyclic polytopes

comment:24 Changed 4 years ago by moritz

thanks, JP!

I moved the text before the cubes to the cyclic example.

comment:25 Changed 4 years ago by jipilab

  • Status changed from needs_review to positive_review

It now looks good to me. Positive review.

comment:26 Changed 4 years ago by vbraun

  • Branch changed from u/moritz/neighborly to eea121525d892277a6706ff9297fafc527105c7b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.