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

Implement the class CombinatorialPolyhedron #26887

Closed
kliem opened this issue Dec 13, 2018 · 332 comments
Closed

Implement the class CombinatorialPolyhedron #26887

kliem opened this issue Dec 13, 2018 · 332 comments

Comments

@kliem
Copy link
Contributor

kliem commented Dec 13, 2018

We construct a class CombinatorialPolyhedron, which collects several methods for polyhedra depending only on the combinatorial type. Actually, most of the methods will work for atomic, coatomic Eulerian lattices.

Computations can be much faster when avoiding creating explicitly the face lattice.

Example:

P = polytopes.permutahedron(6)
P.f_vector()

On my machine this takes over 8s, but it should be a matter of ms. For permutahedron(7) it takes forever and should also be a matter of ms.

Additionally, one can create the face_lattice much faster. The face_lattice of permutahedron(7) should only take few seconds. This is done by creating of lists of all faces and then quickly checking inclusions.

The crucial speed up is obtained by creating bit-vectors for the faces, each bit corresponding to a vertex. Intersection of faces is then only bitwise-and and subset check is (A & ~B == 0). Both steps only take one CPU step per 64/128/256 bits/vertices (depending on the processor). Otherwise the algorithm is pretty similar to current algorithm to create the hasse_diagram, except that it avoids double counting (and hence can calculate the f_vector) without an explicit list of all faces.

The ticket will prepare, but not use, SIMD-instructions. Enabling them will be a subject of #27103.

FOLLOW-UPS:

  • implement calculation of entries in the flag-vector, this might be better, than the calculation in the face_lattice
  • Improve the calculation of properties of polytopes that depend on the face-lattice (f-vector,edges, ridges, k-faces in general, edge-graph, etc.)
  • Enable SIMD-instructions.

CC: @stumpc5 @jplab @tscrim

Component: geometry

Keywords: polytopes, FindPoly, f_vector, face lattice, edge graph, ridge graph

Author: Jonathan Kliem

Branch: 39a7099

Reviewer: Jeroen Demeyer, Travis Scrimshaw, Vincent Delecroix

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

@kliem kliem added this to the sage-8.5 milestone Dec 13, 2018
@kliem
Copy link
Contributor Author

kliem commented Dec 13, 2018

@fchapoton
Copy link
Contributor

Commit: d0dba61

@fchapoton
Copy link
Contributor

comment:2

the branch is almost empty (no meaningful changes)


New commits:

d0dba61first push test

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2018

Changed commit from d0dba61 to 3f1600e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2018

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

a596e0cCreated a class CombinatorialType which is a wrapper for a cpp class
3f1600eadded the files

@kliem
Copy link
Contributor Author

kliem commented Dec 19, 2018

Author: Jonathan Kliem

@kliem kliem self-assigned this Dec 19, 2018
@kliem kliem changed the title Improve f_vector etc. for polytopes Improve f_vector, edges, ridges, dimension for polyhedra Dec 20, 2018
@kliem
Copy link
Contributor Author

kliem commented Dec 20, 2018

Changed keywords from polytopes, FindPoly to polytopes, FindPoly, f_vector, face_lattic

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2018

Changed commit from 3f1600e to 03fe21f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2018

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

3b10c24moved the files to the polyhedron folder
03fe21fadded the new files

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2018

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

99eda99Before Renaming CombinatorialType to CombinatorialPolytope
7efe6a2After Renaming CombinatorialType to CombinatorialPolytope
24949d9This should be mostly done now. Some minor changes are missing, also documentation needs to be done'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2018

Changed commit from 03fe21f to 24949d9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2018

Changed commit from 24949d9 to 8c5d21a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2018

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

8c5d21asome minor changes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2019

Changed commit from 8c5d21a to a63c68f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2019

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

a63c68fPreparing a function to get all k-faces

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 10, 2019

Changed commit from a63c68f to 580980b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 10, 2019

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

580980bsome work on generating all k-faces of the polyhedron

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2019

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

591ed26hasse_diagram is mostly working now, it is much faster than the old hasse_diagram of polyhedron

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2019

Changed commit from 580980b to 591ed26

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Jan 11, 2019

Changed keywords from polytopes, FindPoly, f_vector, face_lattic to polytopes, FindPoly, f_vector, face lattice, flag vector, edge graph, ridge grapg

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 12, 2019

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

576905dflag vectors seem to work now

@videlec
Copy link
Contributor

videlec commented Jun 6, 2019

comment:229

Please move any further discussion to new tickets.

@fchapoton
Copy link
Contributor

Changed dependencies from #27292 to none

@vbraun
Copy link
Member

vbraun commented Jun 27, 2019

comment:231

Fails on e.g. Debian 8:

[sagelib-8.8] [ 98/495] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -fPIC -I/home/buildbot/slave/sage_git/build/local/include/python2.7 -c sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc -o build/temp.linux-i686-2.7/sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.o
[sagelib-8.8] In file included from /usr/include/c++/4.9/cstdint:35:0,
[sagelib-8.8]                  from sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc:13:
[sagelib-8.8] /usr/include/c++/4.9/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support is currently experimental, and must be enabled with the -std=c++11 or -std=gnu++11 compiler options.
[sagelib-8.8]  #error This file requires compiler and library support for the \
[sagelib-8.8]   ^
[sagelib-8.8] sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc:27:22: error: ‘is_subset’ declared as an ‘inline’ variable

@kliem
Copy link
Contributor Author

kliem commented Jun 27, 2019

comment:232

As far as I know there are two ways of solving this.

Either use the macro __cplusplus to determine the c++ version and not use inline if it is not available. (This would be a quick fix I guess).

Use -std=c++11 as compile argument.

Any opinions?

Replying to @vbraun:

Fails on e.g. Debian 8:

[sagelib-8.8] [ 98/495] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -fPIC -I/home/buildbot/slave/sage_git/build/local/include/python2.7 -c sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc -o build/temp.linux-i686-2.7/sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.o
[sagelib-8.8] In file included from /usr/include/c++/4.9/cstdint:35:0,
[sagelib-8.8]                  from sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc:13:
[sagelib-8.8] /usr/include/c++/4.9/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support is currently experimental, and must be enabled with the -std=c++11 or -std=gnu++11 compiler options.
[sagelib-8.8]  #error This file requires compiler and library support for the \
[sagelib-8.8]   ^
[sagelib-8.8] sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc:27:22: error: ‘is_subset’ declared as an ‘inline’ variable

@videlec
Copy link
Contributor

videlec commented Jun 27, 2019

comment:233

Replying to @kliem:

As far as I know there are two ways of solving this.

Either use the macro __cplusplus to determine the c++ version and not use inline if it is not available. (This would be a quick fix I guess).

Use -std=c++11 as compile argument.

Any opinions?

Add the -std=c++11 option to the concerned extension

    Extension('sage.geometry.polyhedron.combinatorial_polyhedron.bit_vector_operations.cc',
              sources = ['sage/geometry/polyhedron/combinatorial_polyhedron/bit_vector_operations.cc'],
              extra_compile_args=['-std=c++11']),

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2019

Changed commit from 8765313 to 39a7099

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2019

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

092c916split list_of_faces, fixed imports etc.
46d54b3added documentation and examples to each module
8ee84cecorrect hyperlinks
3869c1fdocumentation fix
938b04cDo not iterate twice for CombinatorialPolyhedron.facets()
e6d5b08added combinatorial face
69e28baimproved docstring in list_of_all_faces
1e2a52bfixed small issues
e298ed0A number of small edits.
39a7099added compile argument -std=c++11

@kliem
Copy link
Contributor Author

kliem commented Jun 27, 2019

comment:235

added compile argument, this should work now (also #27987 fixes already a bug in the unbounded case)

Thanks for all the help, btw.

@vbraun
Copy link
Member

vbraun commented Jul 4, 2019

Changed branch from public/26887 to 39a7099

@fchapoton
Copy link
Contributor

Changed commit from 39a7099 to none

@fchapoton
Copy link
Contributor

comment:239

This triggers a new failing doctest with python3-sage:

sage -t --long src/sage/geometry/polyhedron/combinatorial_polyhedron/base.pyx
**********************************************************************
File "src/sage/geometry/polyhedron/combinatorial_polyhedron/base.pyx", line 937, in sage.geometry.polyhedron.combinatorial_polyhedron.base.CombinatorialPolyhedron.edge_graph
Failed example:
    G.degree()
Expected:
    [4, 3, 4, 3, 4]
Got:
    [3, 4, 4, 3, 4]

@fchapoton
Copy link
Contributor

comment:240

I have made #28153 for the py3 problem, please review.

@kliem
Copy link
Contributor Author

kliem commented Jul 29, 2019

comment:241

Some attributes like bitrep_vertices might have been an unfortunate choice:

As they are accessed by other classes, I'm forced to compute e.g. bitrep_vertices on initialization. If CombinatorialPolyhedron was ever to be a parent of Polyhedron_base (see #10777), this would force a polyhedron to compute the incidence matrix right away.

From my understanding, the decorator @lazy_attribute is not available for extension types directly. I'm thinking of changing those attributes to methods, such that there is a method bitrep_facets and an attribute _bitrep_facets. This way one can later change this to be lazily evaluated.

Any thoughts?

@videlec
Copy link
Contributor

videlec commented Jul 29, 2019

comment:242

Replying to @kliem:

[...]

Please don't post messages waiting for an answer on a closed ticket. Discussion is closed. Open a new ticket and start a discussion there if something is wrong.

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

9 participants