Provide a simple (and generic) way to create immutable vectors and matrices

[mantepse]

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)

CC: @kliem

Component: misc

Keywords: vector, matrix, immutable

Author: Julian Ritter

Branch/Commit: 5e110ad

Reviewer: Jonathan Kliem

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

[sagetrac-nailuj]

Branch: u/nailuj/immutable_vectors_and_matrices

[sagetrac-nailuj]

Author: Julian Ritter

[sagetrac-nailuj]

comment:2

I propose a modification of the vector and matrix constructors to accept immutable=Tre/False as argument. The default behaviour (mutable output) stays the same.

---

New commits:

5533ba8	added immutable switch for vector constructor
1fd1d48	added immutable switch for matrix constructor

[sagetrac-nailuj]

Commit: 1fd1d48

[sagetrac-nailuj]

Changed keywords from none to vector, matrix, immutable

[kliem]

comment:3

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?

[sagetrac-nailuj]

comment:4

Replying to @kliem:

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 and PolynomialSequence having an immutable=True/False switch
• SimplicialComplex accepts both is_mutable and is_immutable
• Constellation accepts mutable.

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 and Mutability
• Matrix_cyclo_dense and LatinSquare, passing the set_immutable command to its associated matrix
• ToricLattice_quotient_element, having a set_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?

[mwageringel]

comment:5

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

[kliem]

comment:6

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

-    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

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.

• 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 @sagetrac-nailuj:

Replying to @kliem:

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 and PolynomialSequence having an immutable=True/False switch
• SimplicialComplex accepts both is_mutable and is_immutable
• Constellation accepts mutable.

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 and Mutability
• Matrix_cyclo_dense and LatinSquare, passing the set_immutable command to its associated matrix
• ToricLattice_quotient_element, having a set_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?

[sagetrac-git]

Changed commit from 1fd1d48 to 1e4ca9f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

192d634	Handled early returns in vector constructor
b057599	Better vector docstring
203e83d	Specified error in the matrix doctest
1e4ca9f	Simplified construction of immutable matrix

[embray]

comment:9

Ticket retargeted after milestone closed

[sagetrac-git]

Changed commit from 1e4ca9f to d4663d5

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d4663d5

Added tests for each early return

[sagetrac-nailuj]

comment:11

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?

[nbruin]

comment:12

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.

[mantepse]

comment:13

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) than immutable=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).

[kliem]

comment:14

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.

[sagetrac-git]

Changed commit from d4663d5 to 5e110ad

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

5e110ad

Typo in matrix doctest

[sagetrac-nailuj]

comment:16

Replying to @kliem:

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 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.

Thanks!

[kliem]

comment:17

LGTM.

[kliem]

Reviewer: Jonathan Kliem

[vbraun]

Changed branch from u/nailuj/immutable_vectors_and_matrices to 5e110ad