Opened 5 months ago

Closed 4 months ago

#31834 closed enhancement (fixed)

Make Hrepresentation of `CombinatorialPolyhedron` bit more consistent

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.4
Component: geometry Keywords: Hrepresentation, combinatorial polyhedron
Cc: yzh Merged in:
Authors: Jonathan Kliem Reviewers: Yuan Zhou, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 3e0229f (Commits, GitHub, GitLab) Commit: 3e0229f2d13cae98944d37ba5f6a1387334cf96f
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (32)

comment:1 Changed 5 months ago by gh-kliem

  • Branch set to public/31834
  • Commit set to b49a061c2960e10fa19d8d322ab10cf42dadd82d

New commits:

b49a061equalities -> equations

comment:2 Changed 5 months ago by git

  • Commit changed from b49a061c2960e10fa19d8d322ab10cf42dadd82d to a7d13f5d8f7eb14dc755f3e4bd41cdfda01e6d8a

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

9dc75c2have equations always last
a7d13f5make Hrep methods of combinatorial face more consistent

comment:3 Changed 5 months ago by gh-kliem

  • Keywords Hrepresentation combinatorial polyhedron added
  • Status changed from new to needs_review

comment:4 follow-up: Changed 5 months ago by 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.

comment:5 Changed 5 months ago by git

  • Commit changed from a7d13f5d8f7eb14dc755f3e4bd41cdfda01e6d8a to 0111d1fa10c593d5be99d961901d3735e2aebc1a

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

d2ee51emeaningful doctest for ambient_H_indices
0111d1fmake add_equalities more stable

comment:6 in reply to: ↑ 4 Changed 5 months ago by 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.

comment:7 Changed 5 months ago by 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.

comment:8 Changed 5 months ago by yzh

  • Reviewers set to Yuan Zhou

comment:9 Changed 5 months ago by git

  • Commit changed from 0111d1fa10c593d5be99d961901d3735e2aebc1a to 78882fda4b40a5d4038f220ebdba1059f0518456

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

78882fdimproved documentation and removed redundant definitions

comment:10 Changed 5 months ago by gh-kliem

Thank you for the quick review.

comment:11 Changed 5 months ago by gh-kliem

  • Status changed from needs_review to positive_review

comment:12 Changed 5 months ago by yzh

  • Status changed from positive_review to needs_work

comment:13 Changed 5 months ago by 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))

comment:14 Changed 5 months ago by 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)

comment:15 Changed 5 months ago by git

  • Commit changed from 78882fda4b40a5d4038f220ebdba1059f0518456 to 38b2513f2ac1748ec5fca26d74e0ffe5bb6b2b58

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

dee20c3pycodestyle
38b2513fix order of equations of faces

comment:16 Changed 5 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:17 Changed 5 months ago by yzh

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

comment:18 Changed 5 months ago by yzh

I meant, why is _lrs_calculation on this ticket?

comment:19 Changed 5 months ago by gh-kliem

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

comment:20 Changed 5 months ago by git

  • Commit changed from 38b2513f2ac1748ec5fca26d74e0ffe5bb6b2b58 to 25de863dde75e7ab0e4997e14d74cfd6ac7e797b

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

25de863pycodestyle and fix order of equations of faces

comment:21 Changed 5 months ago by gh-kliem

Fixed.

comment:22 Changed 5 months ago by yzh

  • Status changed from needs_review to positive_review

Waiting for the status badges to report green.

comment:23 Changed 5 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:24 Changed 5 months ago by gh-kliem

  • Branch changed from public/31834 to public/31834-reb
  • Commit changed from 25de863dde75e7ab0e4997e14d74cfd6ac7e797b to 5dc2c8c294d6a9cbe7bd3e4369f69e37e0db34d9
  • Status changed from needs_work to positive_review

New commits:

5dc2c8cmerge in public/31834

comment:25 Changed 5 months ago by gh-kliem

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

comment:26 Changed 4 months ago by gh-kliem

  • Branch changed from public/31834-reb to public/31834-reb2
  • Commit changed from 5dc2c8c294d6a9cbe7bd3e4369f69e37e0db34d9 to 297792afd93108f916b60674e7a748bad2609087
  • Status changed from positive_review to needs_review

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


New commits:

f1270b0Merge branch 'public/31834-reb' of git://trac.sagemath.org/sage into public/31834-reb2
297792aequalities -> equations for #31821 as well

comment:27 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to needs_work

Needs rebase

comment:28 Changed 4 months ago by git

  • Commit changed from 297792afd93108f916b60674e7a748bad2609087 to 3e0229f2d13cae98944d37ba5f6a1387334cf96f

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

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

comment:29 Changed 4 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:30 Changed 4 months ago by mkoeppe

  • Reviewers changed from Yuan Zhou to Yuan Zhou, Matthias Koeppe
  • Status changed from needs_review to positive_review

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

comment:31 Changed 4 months ago by gh-kliem

Thank you.

comment:32 Changed 4 months ago by vbraun

  • Branch changed from public/31834-reb2 to 3e0229f2d13cae98944d37ba5f6a1387334cf96f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.