Opened 4 years ago

Closed 23 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: 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 nailuj)

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 2 years ago by nailuj

  • Branch set to u/nailuj/immutable_vectors_and_matrices

comment:2 Changed 2 years ago by nailuj

  • Authors set to Julian Ritter
  • Cc gh-kliem added
  • Commit set to 1fd1d481b8961aae21d11ef91e904fd47a385793
  • Keywords vector matrix immutable added
  • Milestone changed from sage-8.3 to sage-9.0
  • Status changed from new to needs_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 follow-up: Changed 2 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 ; follow-up: Changed 2 years ago by 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:5 Changed 2 years ago by gh-mwageringel

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 2 years ago by gh-mwageringel (previous) (diff)

comment:6 in reply to: ↑ 4 Changed 2 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 2 years ago by gh-kliem

  • Status changed from needs_info to needs_work

comment:8 Changed 2 years ago by git

  • Commit changed from 1fd1d481b8961aae21d11ef91e904fd47a385793 to 1e4ca9fddc49a2b5074846f3c8428fe1d59bf31c

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 2 years ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:10 Changed 23 months ago by git

  • Commit changed from 1e4ca9fddc49a2b5074846f3c8428fe1d59bf31c to d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3

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

d4663d5Added tests for each early return

comment:11 Changed 23 months ago by nailuj

  • 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 follow-up: Changed 23 months ago by 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.

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 23 months ago by nbruin (previous) (diff)

comment:13 in reply to: ↑ 12 Changed 23 months ago by mantepse

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 follow-up: Changed 23 months 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 23 months ago by git

  • Commit changed from d4663d5b098a0dc8b919cdc9b6c93ba3d9f09aa3 to 5e110add6a639a56aa5a31c657c686caa043ebb7

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

5e110adTypo in matrix doctest

comment:16 in reply to: ↑ 14 Changed 23 months ago by nailuj

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 23 months ago by gh-kliem

  • Reviewers set to Jonathan Kliem
  • Status changed from needs_review to positive_review

LGTM.

comment:18 Changed 23 months ago by vbraun

  • Branch changed from u/nailuj/immutable_vectors_and_matrices to 5e110add6a639a56aa5a31c657c686caa043ebb7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.