Matrix class __init__ for sparse matrices is incorrectly documented

[darijgr]

It claims to take a list of triples (i, j, entry in row i and column j), but it actually takes a dictionary (i, j): entry in row i and column j.

I have fixed the error where it appears (integer, rational and mod-n matrices). Are there any other classes where this doc makes sense?

EDIT: Now that I am looking at this again, it worries me that matrix classes for matrices over QQ, ZZ and Zmod(n) ignore the coerce and copy attributes. The idea might be that integers, rationals and ints mod n do not need to be coerced -- but I don't think this is the case (particularly ints mod n); and that integers, rationals and ints mod n do not need to be copied because they are already immutable -- but the copy attribute does not copy the entries, but copies the list/dict of entries, and that is always mutable. This is not fixed here.

CC: @williamstein

Component: linear algebra

Keywords: matrices

Author: Darij Grinberg

Branch/Commit: 37bd776

Reviewer: Jeroen Demeyer

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

bdd70b3	Merge branch 'public/matrix/doc-input-matrix-class' of git://trac.sagemath.org/sage into matrix
a74bc0c	also fix doc in matrix_generic_sparse.py

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from fc58e34 to a74bc0c

[darijgr]

comment:3

Now that I am looking at this again, it worries me that matrix classes for matrices over QQ, ZZ and Zmod(n) ignore the coerce and copy attributes. The idea might be that integers, rationals and ints mod n do not need to be coerced -- but I don't think this is the case (particularly ints mod n); and that integers, rationals and ints mod n do not need to be copied because they are already immutable -- but the copy attribute does not copy the entries, but copies the list/dict of entries, and that is always mutable.

[darijgr]

Description changed:

--- 
+++ 
@@ -1,3 +1,6 @@
 It claims to take a list of triples `(i, j, entry in row i and column j)`, but it actually takes a dictionary `(i, j): entry in row i and column j`.

-I have fixed the error where it appears (integer, rational and mod-n matrices). It would also be good to copy this documentation to other classes where it makes sense.
+I have fixed the error where it appears (integer, rational and mod-n matrices). Are there any other classes where this doc makes sense?
+
+EDIT: Now that I am looking at this again, it worries me that matrix classes for matrices over QQ, ZZ and Zmod(n) ignore the `coerce` and `copy` attributes. The idea might be that integers, rationals and ints mod n do not need to be coerced -- but I don't think this is the case (particularly ints mod n); and that integers, rationals and ints mod n do not need to be copied because they are already immutable -- but the `copy` attribute does not copy the entries, but copies the *list/dict* of entries, and that is always mutable.
+This is not fixed here.

[mezzarobba]

Work Issues: merge conflicts

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a74bc0c to 2ad6f51

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2ad6f51

merge conflict resolved

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2ad6f51 to d4ad7d9

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

d4ad7d9

conflict resolution corrected

[darijgr]

Changed work issues from merge conflicts to none

[darijgr]

comment:10

Fixed.

[jdemeyer]

comment:11

Replying to @darijgr:

Now that I am looking at this again, it worries me that matrix classes for matrices over QQ, ZZ and Zmod(n) ignore the coerce and copy attributes.

They are ignored indeed, but it the safe direction: the code acts if both copy and coerce are True.

[jdemeyer]

Reviewer: Jeroen Demeyer

[darijgr]

comment:12

Thank you!

[mezzarobba]

comment:13

I get doc building errors, due I think to

+        - ``entries`` -- * a Python dictionary whose items have the
+                           form ``(i, j): x``, where ``0 <= i < nrows``,
+                           ``0 <= j < ncols``, and ``x`` is coercible to
+                           an integer.  The ``i,j`` entry of ``self`` is
+                           set to ``x``.  The ``x``'s can be ``0``.
+                         * Alternatively, entries can be a list of *all*
+                           the entries of the sparse matrix, read
+                           row-by-row from top to bottom (so they would
+                           be mostly 0).

and similar changes.

[tscrim]

comment:14

That should be something like this:

        - ``entries`` -- can be one of the following:

          * a Python dictionary whose items have the
            form ``(i, j): x``, where ``0 <= i < nrows``,
            ``0 <= j < ncols``, and ``x`` is coercible to
            an integer.  The ``i,j`` entry of ``self`` is
            set to ``x``.  The ``x``'s can be ``0``.
          * Alternatively, entries can be a list of *all*
            the entries of the sparse matrix, read
            row-by-row from top to bottom (so they would
            be mostly 0).

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

37bd776

doc fixed

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d4ad7d9 to 37bd776

[darijgr]

comment:16

Fixed (and one typo too); thank you!

[vbraun]

Changed branch from public/matrix/doc-input-matrix-class to 37bd776