Opened 8 years ago

Closed 8 years ago

#17245 closed defect (fixed)

Matrix class __init__ for sparse matrices is incorrectly documented

Reported by: Darij Grinberg Owned by:
Priority: minor Milestone: sage-6.4
Component: linear algebra Keywords: matrices, documentation,
Cc: William Stein Merged in:
Authors: Darij Grinberg Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 37bd776 (Commits, GitHub, GitLab) Commit: 37bd776aebdcb2f997f4ab2598275a2b2b97c2c2
Dependencies: Stopgaps:

Status badges

Description (last modified by Darij Grinberg)

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.

Change History (17)

comment:1 Changed 8 years ago by Darij Grinberg

Component: PLEASE CHANGElinear algebra
Status: newneeds_review

comment:2 Changed 8 years ago by git

Commit: fc58e34e949db85332f9c95c12b65e0392224344a74bc0cc7b98f429d69c4bdc842bcb3866ff077e

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

bdd70b3Merge branch 'public/matrix/doc-input-matrix-class' of git://trac.sagemath.org/sage into matrix
a74bc0calso fix doc in matrix_generic_sparse.py

comment:3 Changed 8 years ago by Darij Grinberg

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.

comment:4 Changed 8 years ago by Darij Grinberg

Cc: William Stein added

comment:5 Changed 8 years ago by Darij Grinberg

Description: modified (diff)

comment:6 Changed 8 years ago by Marc Mezzarobba

Status: needs_reviewneeds_work
Work issues: merge conflicts

comment:7 Changed 8 years ago by git

Commit: a74bc0cc7b98f429d69c4bdc842bcb3866ff077e2ad6f51d59f2ada78d692e40c3ce96cb99de3d2e

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

2ad6f51merge conflict resolved

comment:8 Changed 8 years ago by git

Commit: 2ad6f51d59f2ada78d692e40c3ce96cb99de3d2ed4ad7d91a103cea65a9bf1b9e656ea2ef073544e

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

d4ad7d9conflict resolution corrected

comment:9 Changed 8 years ago by Darij Grinberg

Status: needs_workneeds_review
Work issues: merge conflicts

comment:10 Changed 8 years ago by Darij Grinberg

Fixed.

comment:11 in reply to:  3 Changed 8 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer
Status: needs_reviewpositive_review

Replying to darij:

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.

comment:12 Changed 8 years ago by Darij Grinberg

Thank you!

comment:13 Changed 8 years ago by Marc Mezzarobba

Status: positive_reviewneeds_work

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.

comment:14 Changed 8 years ago by Travis Scrimshaw

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

comment:15 Changed 8 years ago by git

Commit: d4ad7d91a103cea65a9bf1b9e656ea2ef073544e37bd776aebdcb2f997f4ab2598275a2b2b97c2c2

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

37bd776doc fixed

comment:16 Changed 8 years ago by Darij Grinberg

Status: needs_workpositive_review

Fixed (and one typo too); thank you!

comment:17 Changed 8 years ago by Volker Braun

Branch: public/matrix/doc-input-matrix-class37bd776aebdcb2f997f4ab2598275a2b2b97c2c2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.