Opened 3 years ago
Closed 20 months ago
#25509 closed enhancement (fixed)
Provide a simple (and generic) way to create immutable vectors and matrices
Reported by:  mantepse  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 22 months ago by
 Branch set to u/nailuj/immutable_vectors_and_matrices
comment:2 Changed 22 months ago by
 Cc ghkliem added
 Commit set to 1fd1d481b8961aae21d11ef91e904fd47a385793
 Keywords vector matrix immutable added
 Milestone changed from sage8.3 to sage9.0
 Status changed from new to needs_info
comment:3 followup: ↓ 4 Changed 22 months 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 in reply to: ↑ 3 ; followup: ↓ 6 Changed 22 months 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 22 months 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 in reply to: ↑ 4 Changed 22 months 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 22 months ago by
 Status changed from needs_info to needs_work
comment:8 Changed 22 months ago by
 Commit changed from 1fd1d481b8961aae21d11ef91e904fd47a385793 to 1e4ca9fddc49a2b5074846f3c8428fe1d59bf31c
comment:9 Changed 21 months ago by
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:10 Changed 20 months ago by
 Commit changed from 1e4ca9fddc49a2b5074846f3c8428fe1d59bf31c to d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3
Branch pushed to git repo; I updated commit sha1. New commits:
d4663d5  Added tests for each early return

comment:11 Changed 20 months ago by
 Description modified (diff)
 Status changed from needs_work to 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 20 months 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 in reply to: ↑ 12 Changed 20 months 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 20 months 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 20 months ago by
 Commit changed from d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3 to 5e110add6a639a56aa5a31c657c686caa043ebb7
Branch pushed to git repo; I updated commit sha1. New commits:
5e110ad  Typo in matrix doctest

comment:16 in reply to: ↑ 14 Changed 20 months 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 20 months ago by
 Reviewers set to Jonathan Kliem
 Status changed from needs_review to positive_review
LGTM.
comment:18 Changed 20 months ago by
 Branch changed from u/nailuj/immutable_vectors_and_matrices to 5e110add6a639a56aa5a31c657c686caa043ebb7
 Resolution set to fixed
 Status changed from positive_review to 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