Opened 2 years ago

Closed 22 months ago

#28616 closed enhancement (fixed)

CombinatorialFace: replace Vrepr() and Hrepr() by more consistent names

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords:
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Laith Rastanawi
Report Upstream: N/A Work issues:
Branch: 184784c (Commits, GitHub, GitLab) Commit: 184784ca2019073d97147864188f3a36220754db
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

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 gh-kliem

  • Description modified (diff)

comment:2 Changed 2 years ago by gh-kliem

  • Dependencies set to #28613

comment:3 Changed 2 years ago by gh-kliem

  • Branch set to public/28616
  • Cc jipilab gh-LaisRast added
  • Commit set to 608ead05d26e4364c296afdf2a04fa73b9d9a466
  • Status changed from new to needs_review

New commits:

d597ed3replace attributes by methods
2fc4fe0removed empty folder being created in source
37592f9replace attributes by methods; remove empty folder from source
e865f9dremoved attribute Vinv, as its not being used
84ef31badded docstrings to the new methods
588afa4removed method for Vinv
a366387H -> facet_names; V -> Vrep
cf8dc4cVrepr(True) -> ambient_Vrepresentation(); Vrepr(False) -> ambient_V_indices
608ead0Hrepr(True) -> ambient_Hrepresentation(); Hrepr(False) -> ambient_H_indices

comment:4 Changed 2 years ago by git

  • Commit changed from 608ead05d26e4364c296afdf2a04fa73b9d9a466 to 531ae53f9a9f5465aa6183c31ef0c04a6bfd1107

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

531ae53changed stacklevel to 3, so deprecation warning shows during normal usage

comment:5 Changed 2 years ago by gh-kliem

  • Status changed from needs_review to needs_work

Waiting on the depend tickets.

comment:6 Changed 2 years ago by gh-kliem

  • Branch changed from public/28616 to public/28616-reb
  • Commit changed from 531ae53f9a9f5465aa6183c31ef0c04a6bfd1107 to 95048a54837130af819abf95b2616f3b6e68a0f4

New commits:

ac4fd9dH -> facet_names; V -> Vrep
7a75db7Vrepr(True) -> ambient_Vrepresentation(); Vrepr(False) -> ambient_V_indices
847bafaHrepr(True) -> ambient_Hrepresentation(); Hrepr(False) -> ambient_H_indices
95048a5changed stacklevel to 3, so deprecation warning shows during normal usage

comment:7 Changed 2 years ago by gh-kliem

  • 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 gh-kliem

  • Branch changed from public/28616-reb to public/28616-reb2
  • Commit changed from 95048a54837130af819abf95b2616f3b6e68a0f4 to cf3e5929e0a9cd1095bb8eef14f02ccb5e05eb90

New commits:

c967cd0Merge branch 'public/28616-reb' of git://trac.sagemath.org/sage into public/28616-reb2
cf3e592applied change of names in facet graph

comment:9 Changed 23 months ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:10 Changed 23 months ago by git

  • Commit changed from cf3e5929e0a9cd1095bb8eef14f02ccb5e05eb90 to 8e2750c6bed82a15eb6a22fce452a166601d9da2

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

8e2750cfixed block style

comment:11 Changed 23 months ago by gh-kliem

  • Dependencies #28613 deleted

comment:12 Changed 23 months ago by gh-LaisRast

  • 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 and ambient_Vrepresentation consistent:

In ambient_Hrepresentation you have

        It consists of the facets/inequalities that contain the face
        and the equalities defining the ambient polyhedron.

In ambient_Vrepresentation you have

        It consists of the ``[vertices, rays, lines]``
        that face contains.
  • Make the documentation of n_ambient_Vrepresentation consistent with the documentation of n_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 git

  • Commit changed from 8e2750c6bed82a15eb6a22fce452a166601d9da2 to b9c6c69c21446439381a6348c423a83e8b8ecae0

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

b9c6c69small improvements to documentation

comment:14 Changed 23 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:15 Changed 23 months ago by gh-LaisRast

  • Status changed from needs_review to positive_review

comment:16 Changed 23 months ago by vbraun

  • 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 gh-kliem

  • Branch changed from public/28616-reb2 to public/28616-reb3
  • Commit changed from b9c6c69c21446439381a6348c423a83e8b8ecae0 to 184784ca2019073d97147864188f3a36220754db
  • Status changed from needs_work to needs_review

I forgot that #28646 (closed now) uses Vrepr and Hrepr.

Applied the changes now. Tests pass on my end.


New commits:

012129cMerge branch 'public/28616-reb2' of git://trac.sagemath.org/sage into public/28616-reb3
184784capplied changes to new function from #28646

comment:18 Changed 23 months ago by gh-LaisRast

  • Status changed from needs_review to positive_review

Now it is good to go

comment:19 Changed 22 months ago by vbraun

  • Branch changed from public/28616-reb3 to 184784ca2019073d97147864188f3a36220754db
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.