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: replace attributes by methods #28605

Closed
kliem opened this issue Oct 15, 2019 · 34 comments
Closed

CombinatorialPolyhedron: replace attributes by methods #28605

kliem opened this issue Oct 15, 2019 · 34 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 15, 2019

Replace attributes in CombinatorialPolyhedron by methods, such that they can potentially be lazily evaluated.

More precisely we replace an attribute by a private attribute (with leading _) and add a method without leading _. E.g. the attribute far_face_tuple is replaced by _far_face_tuple and we add a method far_face_tuple(self). Thus we gain flexibility in the sense that those attributes must not be set on initialization.

This is motivated by #10777.

We remove the attribute Vinv completely, as it is not being used.

CC: @jplab @LaisRast

Component: geometry

Keywords: polytopes, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: d4b3163

Reviewer: Laith Rastanawi, Jean-Philippe Labbé

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

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

kliem commented Oct 15, 2019

Branch: public/28605

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

New commits:

d597ed3replace attributes by methods
2fc4fe0removed empty folder being created in source
37592f9replace attributes by methods; remove empty folder from source

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Commit: 37592f9

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Changed keywords from none to polytopes, combinatorial polyhedron

@kliem

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

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

e865f9dremoved attribute Vinv, as its not being used
84ef31badded docstrings to the new methods
588afa4removed method for Vinv

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

Changed commit from 37592f9 to 588afa4

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

comment:6

Adding #28625 as I'm using attributes in that ticket.

At moment I think #28625 might be finished before this here, but we can always revers dependencies and change it there.

@kliem
Copy link
Contributor Author

kliem commented Oct 18, 2019

Dependencies: #28625

@LaisRast
Copy link

Reviewer: Laith Rastanawi

@LaisRast
Copy link

comment:7

I went through the changes and they seem all legit.
So I will put this on "positive review".

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2019

Changed commit from 588afa4 to 9b5bcaa

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2019

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

b89610eadded combinatorial polyhedron as an attribute for polyhedra
326602cf_vector of CombinatorialPolyhedron is a vector
dfbe2adMerge branch 'public/28607' of git://trac.sagemath.org/sage into public/28621
ed5518bused CombinatorialPolyhedron to compute f_vector
9bdd005give an error message for polytopes in some cases; removed incorrect example
acd671dnow we get a precice error message for inexact truncated dodecahedron
bf85a62subsequent calls for f_vector fail if first attempt fails
dc99ea4Merge branch 'public/28625' of git://trac.sagemath.org/sage into public/28605
9b5bcaaapplied changes of 28605 to new code from 28625

@kliem
Copy link
Contributor Author

kliem commented Oct 19, 2019

comment:10

Failing doctest. Removing .cc to get rid of the empty folder doesn't seem to work.

Should probably just do this in an extra ticket and see what happens then.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2019

Changed commit from 9b5bcaa to 473b15d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2019

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

473b15dundid change to module list

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Oct 21, 2019

comment:12

I think we should to the change to src/module_list.py in another ticket. There is no need to do it now, especially if it leads to problems.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

202cad9replace attributes by methods
2e3e464removed empty folder being created in source
7701062removed attribute Vinv, as its not being used
1b17f6eadded docstrings to the new methods
c51cdd9removed method for Vinv
dd21f9capplied changes of 28605 to new code from 28625
4f49eacundid change to module list

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2019

Changed commit from 473b15d to 4f49eac

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Oct 28, 2019

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

@kliem
Copy link
Contributor Author

kliem commented Oct 28, 2019

Changed commit from 4f49eac to 0f3b121

@kliem
Copy link
Contributor Author

kliem commented Oct 28, 2019

New commits:

0f3b121replace attributes by methods; remove attribute Vinv

@kliem
Copy link
Contributor Author

kliem commented Oct 28, 2019

Changed dependencies from #28625 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2019

Changed commit from 0f3b121 to b0eac08

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b0eac08replace attributes by methods; remove attribute Vinv

@jplab
Copy link

jplab commented Nov 12, 2019

comment:18

Failing doctests on beta5.

@kliem
Copy link
Contributor Author

kliem commented Nov 13, 2019

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

@kliem
Copy link
Contributor Author

kliem commented Nov 13, 2019

Changed commit from b0eac08 to d4b3163

@kliem
Copy link
Contributor Author

kliem commented Nov 13, 2019

New commits:

dd0729dreplace attributes by methods; remove attribute Vinv
d4b3163small fix in ridges

@jplab
Copy link

jplab commented Nov 13, 2019

Changed reviewer from Laith Rastanawi to Laith Rastanawi, Jean-Philippe Labbé

@jplab
Copy link

jplab commented Nov 13, 2019

comment:20

Ok! Looks good to me now.

@vbraun
Copy link
Member

vbraun commented Nov 16, 2019

Changed branch from public/28605-reb2 to d4b3163

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

4 participants