Make Hrepresentation of `CombinatorialPolyhedron` bit more consistent

[gh-kliem]

From #29683.

We change name equalities to equations, which is mostly used for internals and documentation.

We have the equation always last.

We make CombinatorialFace.ambient_H_indices, CombinatorialFace.ambient_Hrepresentation and CombinatorialFace.n_ambient_Hrepresentation consistent.

[gh-kliem]

New commits:

​b49a061

equalities -> equations

[git]

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

​9dc75c2	have equations always last
​a7d13f5	make Hrep methods of combinatorial face more consistent

[yzh]

Looks great to me!

Some small suggestions:

• Doctests of def ambient_H_indices(self, add_equations=True): It would be good to test with add_equations=False and add_equations=True on a polyhedron that has equation (e.g. on the first P = polytopes.permutahedron(5)). The second P =polytopes.cyclic_polytope(4,6) has no equation, so it doesn't show a difference. I'm also curious if add_equations always appends larger indices no matter the backend is field or ppl.
• if add_equations and self._ambient_facets: probably should be two if's, so that not self._ambient_facets and add_equations and self._equations does not result in equations = ()?
• Docstring of Hrepr: the description is unclear, but since it's deprecated, maybe it doesn't matter.

[git]

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

​d2ee51e	meaningful doctest for ambient_H_indices
​0111d1f	make add_equalities more stable

[gh-kliem]

Replying to yzh:

Looks great to me!

Some small suggestions:

• Doctests of def ambient_H_indices(self, add_equations=True): It would be good to test with add_equations=False and add_equations=True on a polyhedron that has equation (e.g. on the first P = polytopes.permutahedron(5)). The second P =polytopes.cyclic_polytope(4,6) has no equation, so it doesn't show a difference.

Fixed.

I'm also curious if add_equations always appends larger indices no matter the backend is field or ppl.

Yes, why wouldn't it. It counts the number of facets and then appends indices. The equations are taken out of the Hrepresentation in base.pyx.

• if add_equations and self._ambient_facets: probably should be two if's, so that not self._ambient_facets and add_equations and self._equations does not result in equations = ()?

Fixed.

• Docstring of Hrepr: the description is unclear, but since it's deprecated, maybe it doesn't matter.

I really don't think it matters. It's deprecated for one hear already and has only been with us for one release total. Only there in case someone used it at some point.

[yzh]

ambient_H_indices:

• Lines 684-685 with add_equations=True?
• Might be good to move the block Add the indices of the equation:: (Lines 677-685) to Line 660--, for clear comparison with the above?
• Line 692 cdef size_t n_facets, n_equations is not used anymore. Remove it?

Everything else looks great to me.

Please feel free to set to Positive review once the Status badges becomes green.

[git]

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

​78882fd

improved documentation and removed redundant definitions

[gh-kliem]

Thank you for the quick review.

[yzh]

While reviewing 29683, I notice that the order of H-indices are disturbed by combinatorial_face_to_polyhedral_face in some backends. The problem seems to be in face.py Lines 855-856:

H_indices = tuple(range(n_ieqs, n_ieqs + n_equations))
H_indices += tuple(x for x in combinatorial_face.ambient_H_indices(add_equations=False))

I think it should be

H_indices = tuple(x for x in combinatorial_face.ambient_H_indices(add_equations=False))
H_indices += tuple(range(n_ieqs, n_ieqs + n_equations))

[yzh]

Bug example:

sage: P = polytopes.permutahedron(3, backend='field')                                                      
sage: CP = CombinatorialPolyhedron(P)                                                                                                                                                       
sage: cf = next(CP.face_iter())                                                                            
sage: cf.ambient_H_indices()                                                                               
(5, 6)
sage: pf = combinatorial_face_to_polyhedral_face(P, cf)                                                    
sage: pf.ambient_H_indices()                                                                               
(6, 5)

The expected output is (5, 6)

[git]

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

​dee20c3	pycodestyle
​38b2513	fix order of equations of faces

[yzh]

Is def _lrs_calculation(self, verbose=False, Vrep=True, goal='volume') a replacement for def _volume_lrs? I don't quite get it.

[yzh]

I meant, why is _lrs_calculation on this ticket?

[gh-kliem]

It was an experimental thing. That's what you get, when you do commit -am. Sorry about that.

[git]

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

​25de863

pycodestyle and fix order of equations of faces

[gh-kliem]

Fixed.

[yzh]

Waiting for the status badges to report green.

[vbraun]

Merge conflict

[gh-kliem]

New commits:

​5dc2c8c

merge in public/31834

[gh-kliem]

Trivial merge conflict because of whitespaces in polyhedron/base.py, which were modified by a different ticket.

[gh-kliem]

#31821 adds a new method __eq__, which needs to be fixed.

---

New commits:

​f1270b0	Merge branch 'public/31834-reb' of git://trac.sagemath.org/sage into public/31834-reb2
​297792a	equalities -> equations for #31821 as well

[mkoeppe]

Needs rebase

[git]

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

​3e0229f

Merge tag '9.4.beta3' into t/31834/public/31834-reb2

[mkoeppe]

This was already positively reviewed. The current version works and LGTM.

[gh-kliem]

Thank you.