Opened 12 months ago
Closed 11 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: |
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 12 months ago by
- Branch set to public/31834
- Commit set to b49a061c2960e10fa19d8d322ab10cf42dadd82d
comment:2 Changed 12 months ago by
- Commit changed from b49a061c2960e10fa19d8d322ab10cf42dadd82d to a7d13f5d8f7eb14dc755f3e4bd41cdfda01e6d8a
comment:3 Changed 12 months ago by
- Keywords Hrepresentation combinatorial polyhedron added
- Status changed from new to needs_review
comment:4 follow-up: ↓ 6 Changed 12 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 12 months ago by
- Commit changed from a7d13f5d8f7eb14dc755f3e4bd41cdfda01e6d8a to 0111d1fa10c593d5be99d961901d3735e2aebc1a
comment:6 in reply to: ↑ 4 Changed 12 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 12 months ago by
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 12 months ago by
- Reviewers set to Yuan Zhou
comment:9 Changed 12 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 12 months ago by
Thank you for the quick review.
comment:11 Changed 12 months ago by
- Status changed from needs_review to positive_review
comment:12 Changed 12 months ago by
- Status changed from positive_review to needs_work
comment:13 Changed 12 months ago by
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 12 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 12 months ago by
- Commit changed from 78882fda4b40a5d4038f220ebdba1059f0518456 to 38b2513f2ac1748ec5fca26d74e0ffe5bb6b2b58
comment:16 Changed 12 months ago by
- Status changed from needs_work to needs_review
comment:17 Changed 12 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 12 months ago by
I meant, why is _lrs_calculation
on this ticket?
comment:19 Changed 12 months ago by
It was an experimental thing. That's what you get, when you do commit -am
. Sorry about that.
comment:20 Changed 12 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 12 months ago by
Fixed.
comment:22 Changed 12 months ago by
- Status changed from needs_review to positive_review
Waiting for the status badges to report green.
comment:23 Changed 12 months ago by
- Status changed from positive_review to needs_work
Merge conflict
comment:24 Changed 12 months ago by
- 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:
5dc2c8c | merge in public/31834
|
comment:25 Changed 12 months ago by
Trivial merge conflict because of whitespaces in polyhedron/base.py
, which were modified by a different ticket.
comment:26 Changed 11 months ago by
- Branch changed from public/31834-reb to public/31834-reb2
- Commit changed from 5dc2c8c294d6a9cbe7bd3e4369f69e37e0db34d9 to 297792afd93108f916b60674e7a748bad2609087
- Status changed from positive_review to needs_review
comment:28 Changed 11 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/31834-reb2
|
comment:29 Changed 11 months ago by
- Status changed from needs_work to needs_review
comment:30 Changed 11 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 11 months ago by
Thank you.
comment:32 Changed 11 months ago by
- Branch changed from public/31834-reb2 to 3e0229f2d13cae98944d37ba5f6a1387334cf96f
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
equalities -> equations