Opened 2 years ago

Closed 22 months ago

#28613 closed enhancement (fixed)

CombinatorialPolyhedron: attributes, H -> facet_names; V -> Vrep

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: polytopes, combinatorial polyhedron
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: ac4fd9d (Commits, GitHub, GitLab) Commit: ac4fd9d01ddddf11e262cb55042922717727f075
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

In order to be more meaningful, the name of the attribute _H of CombinatorialPolyhedron is changed to _facet_names. Likewise the corresponding method is renamed.

Also V is renamed Vrep.

Note that in #28608 we change the abbreviation from Vrepr to Vrep.

In CombinatorialFace we alter this change to ambient_Vrep and ambient_facets.

Change History (13)

comment:1 Changed 2 years ago by gh-kliem

  • Dependencies set to #28605

comment:2 Changed 2 years ago by gh-kliem

  • Description modified (diff)
  • Summary changed from CombinatorialPolyhedron: attributes, H -> facet_names to CombinatorialPolyhedron: attributes, H -> facet_names; V -> Vrep

comment:3 Changed 2 years ago by gh-kliem

  • Description modified (diff)

comment:4 Changed 2 years ago by gh-kliem

  • Branch set to public/28613
  • Cc jipilab gh-LaisRast added
  • Commit set to a36638741e4bdbf509fd00a6154f521bd001cf23
  • 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

comment:5 Changed 2 years ago by gh-kliem

I'm starting to doubt that _ambient_Vrep in CombinatorialFace is a good name for the Vrepresentation names of the polyhedron.

We will have ambient_Vrepresentation correspond to the elements of the Vrepresentation that define the face.

Then again in PolyhedronFace its ambient_dim for the dimension of the polyhedron?

Anyway, I don't think it's a good choice to have _ambient_Vrep and ambient_Vrepresentation mean two different things. What would be a good alternative for _ambient_Vrep?

comment:6 Changed 2 years ago by gh-kliem

  • Status changed from needs_review to needs_work

Waiting on #28605 and will then pull in the changes.

comment:7 Changed 2 years ago by gh-kliem

  • Branch changed from public/28613 to public/28613-reb
  • Commit changed from a36638741e4bdbf509fd00a6154f521bd001cf23 to aeab0770d8f965e7b18bedef3d6266f6c42ab3c5

New commits:

aeab077H -> facet_names; V -> Vrep

comment:8 Changed 2 years ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:9 Changed 2 years ago by gh-kliem

  • Dependencies #28605 deleted

comment:10 Changed 23 months ago by gh-kliem

  • Status changed from needs_review to needs_work

needs to be rebased

comment:11 Changed 23 months ago by gh-kliem

  • Branch changed from public/28613-reb to public/28613-reb2
  • Commit changed from aeab0770d8f965e7b18bedef3d6266f6c42ab3c5 to ac4fd9d01ddddf11e262cb55042922717727f075
  • Status changed from needs_work to needs_review

New commits:

ac4fd9dH -> facet_names; V -> Vrep

comment:12 Changed 23 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. It definitely is a better to have more meaningful name.

comment:13 Changed 22 months ago by vbraun

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