Opened 2 years ago
Closed 22 months ago
#28616 closed enhancement (fixed)
CombinatorialFace: replace Vrepr() and Hrepr() by more consistent names
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  geometry  Keywords:  
Cc:  jipilab, ghLaisRast  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Laith Rastanawi 
Report Upstream:  N/A  Work issues:  
Branch:  184784c (Commits, GitHub, GitLab)  Commit:  184784ca2019073d97147864188f3a36220754db 
Dependencies:  Stopgaps: 
Description (last modified by )
In order to make CombinatorialFace
be more consistent with PolyhedronFace
we make the following changes:
face.Hrepr(True)
>face.ambient_Hrepresentation()
,face.Hrepr(False)
>face.ambient_H_indices()
,face.Vrepr(True)
>face.ambient_Vrepresentation()
,face.Vrepr(False)
>face.ambient_V_indices()
.
We keep the old methods with deprecation warnings.
Change History (19)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Dependencies set to #28613
comment:3 Changed 2 years ago by
 Branch set to public/28616
 Cc jipilab ghLaisRast added
 Commit set to 608ead05d26e4364c296afdf2a04fa73b9d9a466
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Commit changed from 608ead05d26e4364c296afdf2a04fa73b9d9a466 to 531ae53f9a9f5465aa6183c31ef0c04a6bfd1107
Branch pushed to git repo; I updated commit sha1. New commits:
531ae53  changed stacklevel to 3, so deprecation warning shows during normal usage

comment:5 Changed 2 years ago by
 Status changed from needs_review to needs_work
Waiting on the depend tickets.
comment:6 Changed 2 years ago by
 Branch changed from public/28616 to public/28616reb
 Commit changed from 531ae53f9a9f5465aa6183c31ef0c04a6bfd1107 to 95048a54837130af819abf95b2616f3b6e68a0f4
comment:7 Changed 2 years ago by
 Status changed from needs_work to needs_review
As it is, the ticket is ready to be reviewed. Probably it is easier though to wait for #28613 to be merged.
comment:8 Changed 2 years ago by
 Branch changed from public/28616reb to public/28616reb2
 Commit changed from 95048a54837130af819abf95b2616f3b6e68a0f4 to cf3e5929e0a9cd1095bb8eef14f02ccb5e05eb90
comment:9 Changed 23 months ago by
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:10 Changed 23 months ago by
 Commit changed from cf3e5929e0a9cd1095bb8eef14f02ccb5e05eb90 to 8e2750c6bed82a15eb6a22fce452a166601d9da2
Branch pushed to git repo; I updated commit sha1. New commits:
8e2750c  fixed block style

comment:11 Changed 23 months ago by
 Dependencies #28613 deleted
comment:12 Changed 23 months ago by
 Reviewers set to Laith Rastanawi
 Status changed from needs_review to needs_work
The code looks fine. Some remarks on the documentation:
 Make the documentation in
ambient_Hrepresentation
andambient_Vrepresentation
consistent:
In
ambient_Hrepresentation
you haveIt consists of the facets/inequalities that contain the face and the equalities defining the ambient polyhedron.
In
ambient_Vrepresentation
you haveIt consists of the ``[vertices, rays, lines]`` that face contains.
 Make the documentation of
n_ambient_Vrepresentation
consistent with the documentation ofn_ambient_Hrepresentation
: Return the length of the face. +++ Returns the length of the :meth:`CombinatorialFace.ambient_V_indices`.
comment:13 Changed 23 months ago by
 Commit changed from 8e2750c6bed82a15eb6a22fce452a166601d9da2 to b9c6c69c21446439381a6348c423a83e8b8ecae0
Branch pushed to git repo; I updated commit sha1. New commits:
b9c6c69  small improvements to documentation

comment:14 Changed 23 months ago by
 Status changed from needs_work to needs_review
comment:15 Changed 23 months ago by
 Status changed from needs_review to positive_review
comment:16 Changed 23 months ago by
 Status changed from positive_review to needs_work
There are a bunch of test failures that are also on the patchbot
comment:17 Changed 23 months ago by
 Branch changed from public/28616reb2 to public/28616reb3
 Commit changed from b9c6c69c21446439381a6348c423a83e8b8ecae0 to 184784ca2019073d97147864188f3a36220754db
 Status changed from needs_work to needs_review
comment:18 Changed 23 months ago by
 Status changed from needs_review to positive_review
Now it is good to go
comment:19 Changed 22 months ago by
 Branch changed from public/28616reb3 to 184784ca2019073d97147864188f3a36220754db
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
replace attributes by methods
removed empty folder being created in source
replace attributes by methods; remove empty folder from source
removed attribute Vinv, as its not being used
added docstrings to the new methods
removed method for Vinv
H > facet_names; V > Vrep
Vrepr(True) > ambient_Vrepresentation(); Vrepr(False) > ambient_V_indices
Hrepr(True) > ambient_Hrepresentation(); Hrepr(False) > ambient_H_indices