Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate neighborly to combinatorial polyhedron #29565

Closed
kliem opened this issue Apr 24, 2020 · 16 comments
Closed

Migrate neighborly to combinatorial polyhedron #29565

kliem opened this issue Apr 24, 2020 · 16 comments

Comments

@kliem
Copy link
Contributor

kliem commented Apr 24, 2020

We migrate is_neighborly and neighborliness to CombinatorialPolyhedron.
It is also faster now, as use the f-vector instead of indirectly getting the number of k-faces.

Along the way we add is_simplex to CombinatorialPolyhedron and cache the methods f_vector and n_vertices.

CC: @jplab @LaisRast

Component: geometry

Keywords: neighborly polytopes

Author: Jonathan Kliem

Branch/Commit: aa535d8

Reviewer: Jean-Philippe Labbé, Laith Rastanawi

Issue created by migration from https://trac.sagemath.org/ticket/29565

@kliem kliem added this to the sage-9.2 milestone Apr 24, 2020
@kliem
Copy link
Contributor Author

kliem commented Apr 24, 2020

Commit: 847740a

@kliem
Copy link
Contributor Author

kliem commented Apr 24, 2020

Branch: public/29565

@kliem
Copy link
Contributor Author

kliem commented Apr 24, 2020

New commits:

847740amigrate neighborly to combinatorial polyhedron

@LaisRast
Copy link

comment:2
  • I would suggest adding the definitions of "k-neighborly" and "neighborly" instead of referring to Wikipedia.

  • The sentence for the d-simplex is not a definition. It is just an example. So why should it be mentioned?

-        - ``True`` if the every set of up to ``k`` vertices forms a face,
+        - ``True`` if every set of up to ``k`` vertices forms a face

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2020

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

0f74036improvements in the documentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2020

Changed commit from 847740a to 0f74036

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

New commits:

f5caca8migrate neighborly to combinatorial polyhedron
80ce9f9improvements in the documentation

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

Changed branch from public/29565 to public/29565-reb

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

Changed commit from 0f74036 to 80ce9f9

@jplab
Copy link

jplab commented May 11, 2020

Reviewer: Jean-Philippe Labbé

@jplab
Copy link

jplab commented May 11, 2020

comment:6

Small coding style:

+    @cached_method
+    def neighborliness(self):
+        r"""
-        Returns the largest ``k``, such that the polyhedron is ``k``-neighborly.
+        Return the largest ``k``, such that the polyhedron is ``k``-neighborly.

You can put this on positive review on my behalf once this is fixed.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 11, 2020

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

aa535d8coding style

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 11, 2020

Changed commit from 80ce9f9 to aa535d8

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

comment:8

Thank you.


New commits:

aa535d8coding style

New commits:

aa535d8coding style

@LaisRast
Copy link

Changed reviewer from Jean-Philippe Labbé to Jean-Philippe Labbé, Laith Rastanawi

@vbraun
Copy link
Member

vbraun commented May 26, 2020

Changed branch from public/29565-reb to aa535d8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants