_facet_adjacency_matrix not working correctly for non-fulldimensional polyhedra

[cpegel]

The method _facet_adjacency_matrix of the Polyhedron class produces a wrong matrix for polyhedra that are not of same dimension as their ambient space. For example,

sage: s = polytopes.simplex(2)
sage: s._facet_adjacency_matrix()

[0 1 1 1]
[1 0 0 0]
[1 0 0 0]
[1 0 0 0]

while it should return

sage: s._facet_adjacency_matrix()
[0 1 1]
[1 0 1]
[1 1 0]

The problem is that what is being checked is the ambient H-representation of some face being of length 2, which is interpreted as "2 facets are intersecting". This approach doesn't work for codimension not equal to 0.

I have attached a proposed patch that takes codimension into account.

[jipilab]

Is this ready for review?

[cpegel]

Replying to jipilab:

Is this ready for review?

Yes.

[jipilab]

Hi,

• You use P for the polyhedron. It should be self (otherwise no test are going to pass)
• By the same token, you improve the code because you are not computing the face lattice. Therefore the comment in the code should be removed and the description of the ticket (and the title) should be adapted.
• Is it certain that the equations always start the Hrepresentation? This should be a comment in the set_adjacent private function to explain why the indices are shifted.

I am not sure if getting rid of the equations is a good idea, because maybe one would like to use the matrix with the indices given by the H-representation. Perhaps there could be an optional parameter that decides this, and the default could be that the equations are not included.

A last thing, it would be more practical for reviewing and testing that you submit the patch using git.

Instruction can be found here:

​http://doc.sagemath.org/html/en/developer/index.html#git-for-sage-development

Let me know if you have problems or question regarding the procedures.

Thanks for the patch.

[jipilab]

Oh, and you should provide your full name in the "Author" field.

Thanks!

[cpegel]

proposed patch

[cpegel]

Replying to jipilab:

• You use P for the polyhedron. It should be self (otherwise no test are going to pass)

Right, I forgot to change P to self since I was just using this as a script local replacement when testing, thanks.

• By the same token, you improve the code because you are not computing the face lattice. Therefore the comment in the code should be removed and the description of the ticket (and the title) should be adapted.

Since the faces method of the Polyhedron_base class calls self.face_lattice(), this still computes the whole face lattice. So it isn't an improvement regarding this.

• Is it certain that the equations always start the Hrepresentation? This should be a comment in the set_adjacent private function to explain why the indices are shifted.

I guess the best idea is to filter the Hrepresentation for inequalities, I'll try how this works out.

I am not sure if getting rid of the equations is a good idea, because maybe one would like to use the matrix with the indices given by the H-representation. Perhaps there could be an optional parameter that decides this, and the default could be that the equations are not included.

I guess this is really the question of what facet_adjacency_matrix is expected to return. In my opinion: a matrix with numbers of rows/columns equal to the number of facets, in order as they appear in Hrepresentation. Sure it could have a parameter to get a matrix of size the same as the length of Hrepresentation, what would be useful values in the corresponding rows/columns?

A last thing, it would be more practical for reviewing and testing that you submit the patch using git.

Instruction can be found here:

​http://doc.sagemath.org/html/en/developer/index.html#git-for-sage-development

Let me know if you have problems or question regarding the procedures.

Alright, I'll look into it, thanks a lot.

Thanks for the patch.

Thanks for your careful review!

[jipilab]

I added a git branch containing the changes in the patch file.

---

New commits:

​371749e

Applied patch

[vdelecroix]

doctest? There must be an example of what used to fail.

[git]

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

​3eb3711

Added a doctest

[jipilab]

ping!

[git]

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

​fb5bf91

Merge branch 'u/jipilab/22455' of trac.sagemath.org:sage into 22455

[chapoton]

ok

[jipilab]

Cool! Merci!