Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kliem opened this issue Oct 16, 2019 · 19 comments
Closed

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

kliem opened this issue Oct 16, 2019 · 19 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 16, 2019

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.

CC: @jplab @LaisRast

Component: geometry

Keywords: polytopes, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: ac4fd9d

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/28613

@kliem kliem added this to the sage-9.0 milestone Oct 16, 2019
@kliem
Copy link
Contributor Author

kliem commented Oct 16, 2019

Dependencies: #28605

@kliem

This comment has been minimized.

@kliem kliem changed the title CombinatorialPolyhedron: attributes, H -> facet_names CombinatorialPolyhedron: attributes, H -> facet_names; V -> Vrep Oct 16, 2019
@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Oct 16, 2019

Commit: a366387

@kliem
Copy link
Contributor Author

kliem commented Oct 16, 2019

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

@kliem
Copy link
Contributor Author

kliem commented Oct 16, 2019

Branch: public/28613

@kliem
Copy link
Contributor Author

kliem commented Oct 16, 2019

comment:5

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?

@kliem
Copy link
Contributor Author

kliem commented Oct 25, 2019

comment:6

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

@kliem
Copy link
Contributor Author

kliem commented Nov 18, 2019

Changed commit from a366387 to aeab077

@kliem
Copy link
Contributor Author

kliem commented Nov 18, 2019

New commits:

aeab077H -> facet_names; V -> Vrep

@kliem
Copy link
Contributor Author

kliem commented Nov 18, 2019

Changed branch from public/28613 to public/28613-reb

@kliem
Copy link
Contributor Author

kliem commented Nov 18, 2019

Changed dependencies from #28605 to none

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

comment:10

needs to be rebased

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

New commits:

ac4fd9dH -> facet_names; V -> Vrep

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Changed commit from aeab077 to ac4fd9d

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Changed branch from public/28613-reb to public/28613-reb2

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2019

comment:12

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

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2019

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Dec 16, 2019

Changed branch from public/28613-reb2 to ac4fd9d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants