Opened 17 months ago
Closed 16 months ago
#28828 closed defect (fixed)
Attributes of polyhedra are exposed
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  polyhedron, exposed attributes 
Cc:  jipilab, ghLaisRast  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Laith Rastanawi 
Report Upstream:  N/A  Work issues:  
Branch:  a7eeece (Commits, GitHub, GitLab)  Commit:  a7eeece2180be4d91587eb133e7ab73734400f85 
Dependencies:  Stopgaps: 
Description (last modified by )
Currently the f_vector is exposed.
sage: P = polytopes.simplex() sage: P.f_vector()[0] = 2 sage: P.f_vector() (2, 4, 6, 4, 1)
Same applies for
 incidence matrix,
 vertexfacet graph,
 vertices matrix,
 vertex adjacency matrix,
 facet adjacency matrix,
 gale transform.
Some of the above are probably more relevant than others.
Change History (12)
comment:1 Changed 17 months ago by
 Description modified (diff)
 Keywords exposed attributes added; f_vector removed
 Summary changed from f_vector is exposed to Attributes of polyhedra are exposed
comment:2 Changed 16 months ago by
comment:3 Changed 16 months ago by
Thanks for pointing this out.
As for vectors and matrices there is set_immutable
, which should work fine here.
I just didn't get around to doing it.
comment:4 Changed 16 months ago by
 Branch set to public/28828
 Commit set to c1138025fd8a25a48882b483c8bad5399d296ec0
New commits:
c113802  make exposed invariants of polyhedron immutable

comment:5 Changed 16 months ago by
 Status changed from new to needs_review
comment:6 followup: ↓ 10 Changed 16 months ago by
I am not sure we need to change everything to be immutable. But since you are doing this, you may also want to consider the following:
sage: P = polytopes.cube() ....: P.restricted_automorphism_group(output='matrixlist')[0][0,0] = 1000 ....: P.restricted_automorphism_group(output='matrixlist')[0] ....: [1000 0 0 0] [ 0 1 0 0] [ 0 0 1 0] [ 0 0 0 1]
comment:7 Changed 16 months ago by
 Status changed from needs_review to needs_work
comment:8 Changed 16 months ago by
 Commit changed from c1138025fd8a25a48882b483c8bad5399d296ec0 to a7eeece2180be4d91587eb133e7ab73734400f85
comment:9 Changed 16 months ago by
 Status changed from needs_work to needs_review
comment:10 in reply to: ↑ 6 Changed 16 months ago by
As mentioned in the description of the ticket, some methods are worse than others.
E.g. it can easily happen, that one does something with the graph. There is even an example in the doctests (the one I modified in ticket), where the original graph was reversed.
Replying to ghLaisRast:
I am not sure we need to change everything to be immutable. But since you are doing this, you may also want to consider the following:
sage: P = polytopes.cube() ....: P.restricted_automorphism_group(output='matrixlist')[0][0,0] = 1000 ....: P.restricted_automorphism_group(output='matrixlist')[0] ....: [1000 0 0 0] [ 0 1 0 0] [ 0 0 1 0] [ 0 0 0 1]
comment:11 Changed 16 months ago by
 Reviewers set to Laith Rastanawi
 Status changed from needs_review to positive_review
I believe now it is good to go.
comment:12 Changed 16 months ago by
 Branch changed from public/28828 to a7eeece2180be4d91587eb133e7ab73734400f85
 Resolution set to fixed
 Status changed from positive_review to closed
An older open ticket might be worth a look in this context: https://trac.sagemath.org/ticket/25509. It proposes to simplify the creation of immutable vectors/matrices, e.g. by adding an
immutable=True/False
switch to thevector
andmatrix
constructors.