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: sage-9.1
Component: misc Keywords: vector, matrix, immutable
Cc: gh-kliem Merged in:
Authors: Julian Ritter Reviewers: Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: 5e110ad (Commits, GitHub, GitLab) Commit: 5e110add6a639a56aa5a31c657c686caa043ebb7
Dependencies: Stopgaps:

Status badges

Description (last modified by Julian Ritter)

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 Julian Ritter

Branch: u/nailuj/immutable_vectors_and_matrices

comment:2 Changed 3 years ago by Julian Ritter

Authors: Julian Ritter
Cc: gh-kliem added
Commit: 1fd1d481b8961aae21d11ef91e904fd47a385793
Keywords: vector matrix immutable added
Milestone: sage-8.3sage-9.0
Status: newneeds_info

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:

5533ba8added immutable switch for vector constructor
1fd1d48added immutable switch for matrix constructor

comment:3 Changed 3 years ago by gh-kliem

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 ; Changed 3 years ago by Julian Ritter

Replying to gh-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?

comment:5 Changed 3 years ago by Markus Wageringel

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
Last edited 3 years ago by Markus Wageringel (previous) (diff)

comment:6 in reply to:  4 Changed 3 years ago by gh-kliem

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 nailuj:

Replying to gh-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?

comment:7 Changed 3 years ago by gh-kliem

Status: needs_infoneeds_work

comment:8 Changed 3 years ago by git

Commit: 1fd1d481b8961aae21d11ef91e904fd47a3857931e4ca9fddc49a2b5074846f3c8428fe1d59bf31c

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

192d634Handled early returns in vector constructor
b057599Better vector docstring
203e83dSpecified error in the matrix doctest
1e4ca9fSimplified construction of immutable matrix

comment:9 Changed 3 years ago by Erik Bray

Milestone: sage-9.0sage-9.1

Ticket retargeted after milestone closed

comment:10 Changed 3 years ago by git

Commit: 1e4ca9fddc49a2b5074846f3c8428fe1d59bf31cd4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3

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

d4663d5Added tests for each early return

comment:11 Changed 3 years ago by Julian Ritter

Description: modified (diff)
Status: needs_workneeds_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 Changed 3 years ago by Nils Bruin

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.

Last edited 3 years ago by Nils Bruin (previous) (diff)

comment:13 in reply to:  12 Changed 3 years ago by Martin Rubey

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

comment:14 Changed 3 years ago by gh-kliem

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 git

Commit: d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa35e110add6a639a56aa5a31c657c686caa043ebb7

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

5e110adTypo in matrix doctest

comment:16 in reply to:  14 Changed 3 years ago by Julian Ritter

Replying to gh-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!

comment:17 Changed 3 years ago by gh-kliem

Reviewers: Jonathan Kliem
Status: needs_reviewpositive_review

LGTM.

comment:18 Changed 3 years ago by Volker Braun

Branch: u/nailuj/immutable_vectors_and_matrices5e110add6a639a56aa5a31c657c686caa043ebb7
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.