Opened 4 years ago
Closed 3 years ago
#25509 closed enhancement (fixed)
Provide a simple (and generic) way to create immutable vectors and matrices
Reported by:  Martin Rubey  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  misc  Keywords:  vector, matrix, immutable 
Cc:  ghkliem  Merged in:  
Authors:  Julian Ritter  Reviewers:  Jonathan Kliem 
Report Upstream:  N/A  Work issues:  
Branch:  5e110ad (Commits, GitHub, GitLab)  Commit:  5e110add6a639a56aa5a31c657c686caa043ebb7 
Dependencies:  Stopgaps: 
Description (last modified by )
We currently have the following elegant way to produce immutable graphs:
sage: set([Graph(i) for i in range(5)])  TypeError Traceback (most recent call last) ... TypeError: This graph is mutable, and thus not hashable. Create an immutable copy by `g.copy(immutable=True)` sage: set([Graph(i, immutable=True) for i in range(5)]) {Graph on 0 vertices, Graph on 1 vertex, Graph on 2 vertices, Graph on 3 vertices, Graph on 4 vertices}
The aim of this ticket is to have the same for other objects, in particular vectors and matrices.
It would avoid introducing a variable just to make this adjustment. This can simplify code by more than just one line. Consider, for example, list/set/dict comprehensions and compare
S = set() for x in some_list: v = vector(x) v.set_immutable() S.add(v)
to
S = set(vector(x, immutable=True) for x in some_list)
Change History (18)
comment:1 Changed 3 years ago by
Branch:  → u/nailuj/immutable_vectors_and_matrices 

comment:2 Changed 3 years ago by
Authors:  → Julian Ritter 

Cc:  ghkliem added 
Commit:  → 1fd1d481b8961aae21d11ef91e904fd47a385793 
Keywords:  vector matrix immutable added 
Milestone:  sage8.3 → sage9.0 
Status:  new → needs_info 
comment:3 followup: 4 Changed 3 years ago by
I personally do not see the advantage to just setting the matrix/vector immutable, once I'm done modifying it.
Are there other examples (than graphs) that do it with keywords?
comment:4 followup: 6 Changed 3 years ago by
Replying to ghkliem:
I personally do not see the advantage to just setting the matrix/vector immutable, once I'm done modifying it.
It would avoid introducing a variable just to make this adjustment. This can simplify code by more than just one line. Consider, for example, list/set/dict comprehensions and compare
S = set() for x in some_list: v = vector(x) v.set_immutable() S.add(v)
to
S = set(vector(x, immutable=True) for x in some_list)
Are there other examples (than graphs) that do it with keywords?
I did a quick research. Apart from Graph
and DiGraph
, I found:
Sequence
andPolynomialSequence
having animmutable=True/False
switchSimplicialComplex
accepts bothis_mutable
andis_immutable
Constellation
acceptsmutable
.
Apart from vector
and matrix
, the only classes I found having a set_immutable
method, but no corresponding switch in the constructor are the following:
 the abstract classes
ClonableElement
andMutability
Matrix_cyclo_dense
andLatinSquare
, passing theset_immutable
command to its associated matrixToricLattice_quotient_element
, having aset_mutable
method, which "does nothing, it is introduced for compatibility purposes only".
Just like @mantepse, I would prefer vector
and matrix
to have such switches. Is there a striking argument why they shouldn't?
comment:5 Changed 3 years ago by
Thank you for implementing this. Personally, I have been missing this option several times and was surprised it did not exist.
The vector
function has early returns that are not handled yet, so the immutable
keyword is sometimes ignored. Moreover, the docstring should probably be of the form:
 ``immutable``  boolean (default: ``False``); whether the result should be an immutable vector
comment:6 Changed 3 years ago by
Thanks. Especially the example illustrated to me, why such a switch makes sense. Maybe you could add this example to the description of the ticket.
I'm not into matrices or vectors too much (I never contributed to them). So if I'm going to agree to adding a switch, I want to know, that it is useful. I never had a striking reason against adding the immutable
switch, but one needs at least one argument on the pro side as well (otherwise we could add tons of almost useless switches for very special cases).
Some more remarks:
 You should specify the error in the doctest:
 sage: matrix is immutable; please change a copy instead (i.e., use copy(M) to change a copy of M). + ValueError: matrix is immutable; please change a copy instead (i.e., use copy(M) to change a copy of M).
 Maybe you can simplify the construction
The reason I'm suggesting it: It is clearer, what the switch immutable does. It doesn't effect the construction of the matrix, it is just setting the matrix immutable, after the construction.
 if kwds.pop('immutable', False):  M = MatrixArgs(*args, **kwds).matrix()  M.set_immutable()  return M   return MatrixArgs(*args, **kwds).matrix() + immutable = kwds.pop('immutable', False) + M = MatrixArgs(*args, **kwds).matrix() + if immutable: + M.set_immutable() + return M
 I find it strange, when the comments preceding examples end with
. ::
. However, it seems to work well with sphinx: http://doc.sagemath.org/html/en/reference/modules/sage/modules/free_module.html
Otherwise, this looks fine modulo the changes suggested by Markus Wageringel. Maybe one could add tiny tests for each case of the early returns.
Replying to nailuj:
Replying to ghkliem:
I personally do not see the advantage to just setting the matrix/vector immutable, once I'm done modifying it.
It would avoid introducing a variable just to make this adjustment. This can simplify code by more than just one line. Consider, for example, list/set/dict comprehensions and compare
S = set() for x in some_list: v = vector(x) v.set_immutable() S.add(v)to
S = set(vector(x, immutable=True) for x in some_list)Are there other examples (than graphs) that do it with keywords?
I did a quick research. Apart from
Graph
andDiGraph
, I found:
Sequence
andPolynomialSequence
having animmutable=True/False
switchSimplicialComplex
accepts bothis_mutable
andis_immutable
Constellation
acceptsmutable
.Apart from
vector
andmatrix
, the only classes I found having aset_immutable
method, but no corresponding switch in the constructor are the following:
 the abstract classes
ClonableElement
andMutability
Matrix_cyclo_dense
andLatinSquare
, passing theset_immutable
command to its associated matrixToricLattice_quotient_element
, having aset_mutable
method, which "does nothing, it is introduced for compatibility purposes only".Just like @mantepse, I would prefer
vector
andmatrix
to have such switches. Is there a striking argument why they shouldn't?
comment:7 Changed 3 years ago by
Status:  needs_info → needs_work 

comment:8 Changed 3 years ago by
Commit:  1fd1d481b8961aae21d11ef91e904fd47a385793 → 1e4ca9fddc49a2b5074846f3c8428fe1d59bf31c 

comment:9 Changed 3 years ago by
Milestone:  sage9.0 → sage9.1 

Ticket retargeted after milestone closed
comment:10 Changed 3 years ago by
Commit:  1e4ca9fddc49a2b5074846f3c8428fe1d59bf31c → d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3 

Branch pushed to git repo; I updated commit sha1. New commits:
d4663d5  Added tests for each early return

comment:11 Changed 3 years ago by
Description:  modified (diff) 

Status:  needs_work → needs_review 
I made all the proposed adjustments. This should be ready for review.
While working on the early returns, I noticed that v = vector(R, w)
and v = vector(w, R)
just output w
if R
is equal to None
or w.base_ring()
. As a consequence, all changes applied to v
are also applied to w
. Do you think this is the desired behaviour or should a ticket be opened to ask if this might be a bug?
comment:12 followup: 13 Changed 3 years ago by
I don't feel very strongly about this point, but mutable=False
seems like a more direct way (at least in english) than immutable=True
: naming an option directly than by its negation seems more straightforward. Perhaps worth considering.
EDIT: as pointed out, consistency is important and for graphs it looks like immutable
already forces our hand. Changing all to mutable
would be a different discussion and seems not worth it.
comment:13 Changed 3 years ago by
Replying to nbruin:
I don't feel very strongly about this point, but
mutable=False
seems like a more direct way (at least in english) thanimmutable=True
: naming an option directly than by its negation seems more straightforward. Perhaps worth considering.
I think it should be consistent with graphs (either way).
comment:14 followup: 16 Changed 3 years ago by
Your missing the colons after Traceback (most recent call last)
in the doctest. So there is a failing test.
Also I think, error message should not end with a period.
I think returning a
with vector(a)
is definitely not the desired behavior. E.g. list
, matrix
and graph
return a copy in this case. So one would expect vector
to behave the same.
comment:15 Changed 3 years ago by
Commit:  d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3 → 5e110add6a639a56aa5a31c657c686caa043ebb7 

Branch pushed to git repo; I updated commit sha1. New commits:
5e110ad  Typo in matrix doctest

comment:16 Changed 3 years ago by
Replying to ghkliem:
Your missing the colons after
Traceback (most recent call last)
in the doctest. So there is a failing test.
Thank you, I only tested the vector file. I fixed it.
Also I think, error message should not end with a period.
Could you be more specific? I don't think I introduced any error message.
I think returning
a
withvector(a)
is definitely not the desired behavior. E.g.list
,matrix
andgraph
return a copy in this case. So one would expectvector
to behave the same.
Thanks!
comment:17 Changed 3 years ago by
Reviewers:  → Jonathan Kliem 

Status:  needs_review → positive_review 
LGTM.
comment:18 Changed 3 years ago by
Branch:  u/nailuj/immutable_vectors_and_matrices → 5e110add6a639a56aa5a31c657c686caa043ebb7 

Resolution:  → fixed 
Status:  positive_review → closed 
I propose a modification of the
vector
andmatrix
constructors to acceptimmutable=Tre/False
as argument. The default behaviour (mutable output) stays the same.New commits:
added immutable switch for vector constructor
added immutable switch for matrix constructor