Opened 2 years ago
Closed 2 years ago
#28605 closed enhancement (fixed)
CombinatorialPolyhedron: replace attributes by methods
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  polytopes, combinatorial polyhedron 
Cc:  jipilab, ghLaisRast  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Laith Rastanawi, JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  d4b3163 (Commits, GitHub, GitLab)  Commit:  d4b3163762932706ba2af5493e55c4d4c4a58ed0 
Dependencies:  Stopgaps: 
Description (last modified by )
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.
Change History (21)
comment:1 Changed 2 years ago by
 Branch set to public/28605
 Commit set to 37592f9458a1f680f90c6befd4b238558b8d5721
comment:2 Changed 2 years ago by
 Keywords polytopes combinatorial polyhedron added
comment:3 Changed 2 years ago by
 Description modified (diff)
comment:4 Changed 2 years ago by
 Commit changed from 37592f9458a1f680f90c6befd4b238558b8d5721 to 588afa4cf3b700e142de154f97d32ee33795b6a9
comment:5 Changed 2 years ago by
 Cc jipilab ghLaisRast added
 Status changed from new to needs_review
comment:6 Changed 2 years ago by
 Dependencies set to #28625
comment:7 Changed 2 years ago by
 Reviewers set to Laith Rastanawi
 Status changed from needs_review to positive_review
I went through the changes and they seem all legit. So I will put this on "positive review".
comment:8 Changed 2 years ago by
 Commit changed from 588afa4cf3b700e142de154f97d32ee33795b6a9 to 9b5bcaafd61760a75f47451df5febf91d55cff12
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
b89610e  added combinatorial polyhedron as an attribute for polyhedra

326602c  f_vector of CombinatorialPolyhedron is a vector

dfbe2ad  Merge branch 'public/28607' of git://trac.sagemath.org/sage into public/28621

ed5518b  used CombinatorialPolyhedron to compute f_vector

9bdd005  give an error message for polytopes in some cases; removed incorrect example

acd671d  now we get a precice error message for inexact truncated dodecahedron

bf85a62  subsequent calls for f_vector fail if first attempt fails

dc99ea4  Merge branch 'public/28625' of git://trac.sagemath.org/sage into public/28605

9b5bcaa  applied changes of 28605 to new code from 28625

comment:9 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:10 Changed 2 years ago by
 Status changed from positive_review to needs_work
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.
comment:11 Changed 2 years ago by
 Commit changed from 9b5bcaafd61760a75f47451df5febf91d55cff12 to 473b15d1f08eca87314a201477dcc1e2bd286b7c
Branch pushed to git repo; I updated commit sha1. New commits:
473b15d  undid change to module list

comment:12 Changed 2 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
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.
comment:13 Changed 2 years ago by
 Commit changed from 473b15d1f08eca87314a201477dcc1e2bd286b7c to 4f49eac6517be9e46924a6368446ac3697cbd46a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
202cad9  replace attributes by methods

2e3e464  removed empty folder being created in source

7701062  removed attribute Vinv, as its not being used

1b17f6e  added docstrings to the new methods

c51cdd9  removed method for Vinv

dd21f9c  applied changes of 28605 to new code from 28625

4f49eac  undid change to module list

comment:14 Changed 2 years ago by
 Description modified (diff)
comment:15 Changed 2 years ago by
 Branch changed from public/28605 to public/28605reb
 Commit changed from 4f49eac6517be9e46924a6368446ac3697cbd46a to 0f3b121b9f1cebe0c65056bb2fc364e9b770e58f
New commits:
0f3b121  replace attributes by methods; remove attribute Vinv

comment:16 Changed 2 years ago by
 Dependencies #28625 deleted
comment:17 Changed 2 years ago by
 Commit changed from 0f3b121b9f1cebe0c65056bb2fc364e9b770e58f to b0eac0858f113c04beb123e747851982d6447a65
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b0eac08  replace attributes by methods; remove attribute Vinv

comment:18 Changed 2 years ago by
 Status changed from needs_review to needs_work
Failing doctests on beta5.
comment:19 Changed 2 years ago by
 Branch changed from public/28605reb to public/28605reb2
 Commit changed from b0eac0858f113c04beb123e747851982d6447a65 to d4b3163762932706ba2af5493e55c4d4c4a58ed0
 Status changed from needs_work to needs_review
comment:20 Changed 2 years ago by
 Reviewers changed from Laith Rastanawi to Laith Rastanawi, JeanPhilippe Labbé
 Status changed from needs_review to positive_review
Ok! Looks good to me now.
comment:21 Changed 2 years ago by
 Branch changed from public/28605reb2 to d4b3163762932706ba2af5493e55c4d4c4a58ed0
 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