Opened 5 months ago
Closed 4 months ago
#31834 closed enhancement (fixed)
Make Hrepresentation of `CombinatorialPolyhedron` bit more consistent
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.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: 
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
 Branch set to public/31834
 Commit set to b49a061c2960e10fa19d8d322ab10cf42dadd82d
comment:2 Changed 5 months ago by
 Commit changed from b49a061c2960e10fa19d8d322ab10cf42dadd82d to a7d13f5d8f7eb14dc755f3e4bd41cdfda01e6d8a
comment:3 Changed 5 months ago by
 Keywords Hrepresentation combinatorial polyhedron added
 Status changed from new to needs_review
comment:4 followup: ↓ 6 Changed 5 months ago by
Looks great to me!
Some small suggestions:
 Doctests of
def ambient_H_indices(self, add_equations=True)
: It would be good to test withadd_equations=False
andadd_equations=True
on a polyhedron that has equation (e.g. on the firstP = polytopes.permutahedron(5)
). The secondP =polytopes.cyclic_polytope(4,6)
has no equation, so it doesn't show a difference. I'm also curious ifadd_equations
always appends larger indices no matter the backend isfield
orppl
. if add_equations and self._ambient_facets:
probably should be two if's, so thatnot self._ambient_facets and add_equations and self._equations
does not result inequations = ()
? Docstring of
Hrepr
: the description is unclear, but since it's deprecated, maybe it doesn't matter.
comment:5 Changed 5 months ago by
 Commit changed from a7d13f5d8f7eb14dc755f3e4bd41cdfda01e6d8a to 0111d1fa10c593d5be99d961901d3735e2aebc1a
comment:6 in reply to: ↑ 4 Changed 5 months ago by
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 withadd_equations=False
andadd_equations=True
on a polyhedron that has equation (e.g. on the firstP = polytopes.permutahedron(5)
). The secondP =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 isfield
orppl
.
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 thatnot self._ambient_facets and add_equations and self._equations
does not result inequations = ()
?
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
ambient_H_indices
:
 Lines 684685 with
add_equations=True
?  Might be good to move the block
Add the indices of the equation::
(Lines 677685) 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
 Reviewers set to Yuan Zhou
comment:9 Changed 5 months ago by
 Commit changed from 0111d1fa10c593d5be99d961901d3735e2aebc1a to 78882fda4b40a5d4038f220ebdba1059f0518456
Branch pushed to git repo; I updated commit sha1. New commits:
78882fd  improved documentation and removed redundant definitions

comment:10 Changed 5 months ago by
Thank you for the quick review.
comment:11 Changed 5 months ago by
 Status changed from needs_review to positive_review
comment:12 Changed 5 months ago by
 Status changed from positive_review to needs_work
comment:13 Changed 5 months ago by
While reviewing 29683, I notice that the order of Hindices are disturbed by combinatorial_face_to_polyhedral_face
in some backends. The problem seems to be in face.py
Lines 855856:
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
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
 Commit changed from 78882fda4b40a5d4038f220ebdba1059f0518456 to 38b2513f2ac1748ec5fca26d74e0ffe5bb6b2b58
comment:16 Changed 5 months ago by
 Status changed from needs_work to needs_review
comment:17 Changed 5 months ago by
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
I meant, why is _lrs_calculation
on this ticket?
comment:19 Changed 5 months ago by
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
 Commit changed from 38b2513f2ac1748ec5fca26d74e0ffe5bb6b2b58 to 25de863dde75e7ab0e4997e14d74cfd6ac7e797b
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

comment:21 Changed 5 months ago by
Fixed.
comment:22 Changed 5 months ago by
 Status changed from needs_review to positive_review
Waiting for the status badges to report green.
comment:24 Changed 5 months ago by
 Branch changed from public/31834 to public/31834reb
 Commit changed from 25de863dde75e7ab0e4997e14d74cfd6ac7e797b to 5dc2c8c294d6a9cbe7bd3e4369f69e37e0db34d9
 Status changed from needs_work to positive_review
New commits:
5dc2c8c  merge in public/31834

comment:25 Changed 5 months ago by
Trivial merge conflict because of whitespaces in polyhedron/base.py
, which were modified by a different ticket.
comment:26 Changed 4 months ago by
 Branch changed from public/31834reb to public/31834reb2
 Commit changed from 5dc2c8c294d6a9cbe7bd3e4369f69e37e0db34d9 to 297792afd93108f916b60674e7a748bad2609087
 Status changed from positive_review to needs_review
comment:28 Changed 4 months ago by
 Commit changed from 297792afd93108f916b60674e7a748bad2609087 to 3e0229f2d13cae98944d37ba5f6a1387334cf96f
Branch pushed to git repo; I updated commit sha1. New commits:
3e0229f  Merge tag '9.4.beta3' into t/31834/public/31834reb2

comment:29 Changed 4 months ago by
 Status changed from needs_work to needs_review
comment:30 Changed 4 months ago by
 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
Thank you.
comment:32 Changed 4 months ago by
 Branch changed from public/31834reb2 to 3e0229f2d13cae98944d37ba5f6a1387334cf96f
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
equalities > equations