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

Attributes of polyhedra are exposed #28828

Closed
kliem opened this issue Dec 2, 2019 · 16 comments
Closed

Attributes of polyhedra are exposed #28828

kliem opened this issue Dec 2, 2019 · 16 comments

Comments

@kliem
Copy link
Contributor

kliem commented Dec 2, 2019

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,
  • vertex-facet graph,
  • vertices matrix,
  • vertex adjacency matrix,
  • facet adjacency matrix,
  • gale transform.

Some of the above are probably more relevant than others.

CC: @jplab @LaisRast

Component: geometry

Keywords: polyhedron, exposed attributes

Author: Jonathan Kliem

Branch/Commit: a7eeece

Reviewer: Laith Rastanawi

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

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

kliem commented Dec 2, 2019

Changed author from Jonathan Kliem to none

@kliem
Copy link
Contributor Author

kliem commented Dec 2, 2019

Changed keywords from polyhedron, f_vector to polyhedron, exposed attributes

@kliem

This comment has been minimized.

@kliem kliem changed the title f_vector is exposed Attributes of polyhedra are exposed Dec 2, 2019
@sagetrac-nailuj
Copy link
Mannequin

sagetrac-nailuj mannequin commented Dec 10, 2019

comment:2

An older open ticket might be worth a look in this context: #25509. It proposes to simplify the creation of immutable vectors/matrices, e.g. by adding an immutable=True/False switch to the vector and matrix constructors.

@kliem
Copy link
Contributor Author

kliem commented Dec 10, 2019

comment:3

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.

@kliem
Copy link
Contributor Author

kliem commented Dec 13, 2019

Branch: public/28828

@kliem
Copy link
Contributor Author

kliem commented Dec 13, 2019

Commit: c113802

@kliem
Copy link
Contributor Author

kliem commented Dec 13, 2019

New commits:

c113802make exposed invariants of polyhedron immutable

@kliem
Copy link
Contributor Author

kliem commented Dec 13, 2019

Author: Jonathan Kliem

@LaisRast
Copy link
Contributor

comment:6

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]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2019

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

50a4975make exposed invariants of polyhedron immutable
a7eeecemake restricted automorphism group as matrixlist immutable

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2019

Changed commit from c113802 to a7eeece

@kliem
Copy link
Contributor Author

kliem commented Dec 17, 2019

comment:10

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 @LaisRast:

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]

@LaisRast
Copy link
Contributor

Reviewer: Laith Rastanawi

@LaisRast
Copy link
Contributor

comment:11

I believe now it is good to go.

@vbraun
Copy link
Member

vbraun commented Dec 20, 2019

Changed branch from public/28828 to a7eeece

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