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

Wrong f-vector for unbounded polyhedra #26922

Closed
kliem opened this issue Dec 20, 2018 · 21 comments
Closed

Wrong f-vector for unbounded polyhedra #26922

kliem opened this issue Dec 20, 2018 · 21 comments

Comments

@kliem
Copy link
Contributor

kliem commented Dec 20, 2018

#28625 fixed the f_vector in the case of unpointed polyhedra/polyhedra with lines.

We add doctests showing that #28625 fixed a bug in f_vector.

Before:

sage: Polyhedron(ieqs=[[1,-1,0],[1,1,0]]).f_vector()
(1, 2, 1)

But this polyhedron does not have zero-dimensional faces, and #28625 has correctly changed that:

sage: Polyhedron(ieqs=[[1,-1,0],[1,1,0]]).f_vector()
(1, 0, 2, 1)

Also we add documentation, specifically warning users that the methods

  • vertices,
  • vertices_list,
  • vertices_generator
  • vertices_matrix,
  • n_vertices,
    treat vertices of the Vrepresentation and not vertices of the polyhedron in the unpointed case.

Depends on #28625

CC: @jplab @mo271

Component: geometry

Author: Jonathan Kliem

Branch/Commit: 1c5378c

Reviewer: Jean-Philippe Labbé

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

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

kliem commented Jan 12, 2019

comment:1

This will be probably solved at some point by
#26887
The calculation is correct there.

Also it can be corrected quickly now by calculating the dimension of the first level in the face lattice.

@embray
Copy link
Contributor

embray commented Jan 15, 2019

comment:2

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

@embray embray modified the milestones: sage-8.6, sage-8.7 Jan 15, 2019
@jplab jplab modified the milestones: sage-8.7, sage-8.8 Mar 14, 2019
@embray
Copy link
Contributor

embray commented Jun 14, 2019

comment:4

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

@embray embray removed this from the sage-8.8 milestone Jun 14, 2019
@jplab
Copy link
Contributor

jplab commented Jul 22, 2019

comment:5

In principle, the problem is that the face lattice has four elements, but the elements on the second level are not 0-dimensional, but 1-dimensional.

So, depending on the definition of the f-vector:

  • Counting the number of elements at each level of the face lattice
  • Vector counting the number of faces by dimension

It should return different things.

One thing that should be done, is to make this explicit in the documentation of f_vector once this ticket is fixed and give an example of the difference with unbounded polyhedra.

@kliem
Copy link
Contributor Author

kliem commented Jul 22, 2019

comment:6

#27063 will change the calculation of the f-vector (hopefully soon) to make use of CombinatorialPolyhedron.

This will count the number of faces per dimension, as the docstring of f_vector states (there is a +2 or -2 two missing, as the first entry gives the -1-dimensional faces count.

We could make this ticket depend on #27063 and then just append the docstring accordingly.

One could also fix it for now (just append a few zeros according to n_lines).

Replying to @jplab:

In principle, the problem is that the face lattice has four elements, but the elements on the second level are not 0-dimensional, but 1-dimensional.

So, depending on the definition of the f-vector:

  • Counting the number of elements at each level of the face lattice

I think f-vectors of fixed dimension polyhedra should be have sums/addition.
Also if one considers how adding or deleting inequalities can alter the f-vector this is strange.
Anyway, it's most important to be precise, which one we use. Adding a zero or deleting it, if one really depends one version is trivial.

  • Vector counting the number of faces by dimension

It should return different things.

One thing that should be done, is to make this explicit in the documentation of f_vector once this ticket is fixed and give an example of the difference with unbounded polyhedra.

@jplab
Copy link
Contributor

jplab commented Oct 17, 2019

comment:7

ping!

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

Branch: public/26922

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

comment:8

#28625 fixes this.

I added some tests, improved the documentation of f_vector and fixed a tiny typo.


New commits:

b89610eadded combinatorial polyhedron as an attribute for polyhedra
326602cf_vector of CombinatorialPolyhedron is a vector
dfbe2adMerge branch 'public/28607' of git://trac.sagemath.org/sage into public/28621
ed5518bused CombinatorialPolyhedron to compute f_vector
9bdd005give an error message for polytopes in some cases; removed incorrect example
acd671dnow we get a precice error message for inexact truncated dodecahedron
bf85a62subsequent calls for f_vector fail if first attempt fails
3c9085egive doctests that f_vector for unbounded polyhedra works now

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

Author: Jonathan Kliem

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

Commit: 3c9085e

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

Dependencies: #28625

@kliem kliem added this to the sage-9.0 milestone Oct 18, 2019
@jplab
Copy link
Contributor

jplab commented Oct 19, 2019

comment:9

It would be nice if the note that you gave be in a NOTE environment followed by an appropriate example illustrating what is meant.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 22, 2019

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

cbcc4c4Comment to f_vector in NOTE environment; warnings of ambigious meaning of vertex

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 22, 2019

Changed commit from 3c9085e to cbcc4c4

@kliem

This comment has been minimized.

@jplab
Copy link
Contributor

jplab commented Oct 24, 2019

comment:12

May I suggest the following:

-            In case of a polyhedron with lines (unpointed polyhedron),
-            return the number of vertices of the ``Vrepresentation``.
-            Wheras the polyhedron has no vertices, this number corresponds
-            to the number of `k`-faces, where `k` is the number of lines.
+            If the polyhedron has lines, returns the number of vertices in
+            the ``Vrepresentation``. As the represented polyhedron has 
+            no 0-dimensional faces (i.e. vertices), this number corresponds
+            to the number of `k`-faces, where `k` is the number of lines.

Somehow, even though I corrected the above warning. I do not like what it says at all for the following reason:

sage: P = Polyhedron(rays=[[1,0,0]],lines=[[0,1,0]])
sage: P.vertices()
(A vertex at (0, 0, 0),)
sage: Q = Polyhedron(lines=[[0,1,0],[0,0,1]])
sage: Q.vertices()
(A vertex at (0, 0, 0),)
sage: R = Polyhedron(rays=[[1,0,0],[0,1,0]],vertices=[[0,1,0],[1,0,0]],lines=[[0,0,1]])
sage: R.vertices()
(A vertex at (0, 1, 0), A vertex at (1, 0, 0))

In Sage, the computational convention (or compromise) is that polyhedra without vertices in their V-representation still receive a canonically computed vertex in order to do computations.

With this in mind, the second sentence is wrong. Just look at the above polyhedron R. It has lines, and two vertices.

---> This warning is more confusing than anything. Please rephrase taking the above three examples in mind and the convention in Sage.

@jplab
Copy link
Contributor

jplab commented Oct 24, 2019

Reviewer: Jean-Philippe Labbé

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2019

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

1c5378cclearified warnings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2019

Changed commit from cbcc4c4 to 1c5378c

@jplab
Copy link
Contributor

jplab commented Oct 27, 2019

comment:15

Looks good to me now

@vbraun
Copy link
Member

vbraun commented Oct 28, 2019

Changed branch from public/26922 to 1c5378c

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