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:  sage6.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: 
Description (last modified by )
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 modn 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
Component:  PLEASE CHANGE → linear algebra 

Status:  new → needs_review 
comment:2 Changed 8 years ago by
Commit:  fc58e34e949db85332f9c95c12b65e0392224344 → a74bc0cc7b98f429d69c4bdc842bcb3866ff077e 

comment:3 followup: 11 Changed 8 years ago by
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
Cc:  William Stein added 

comment:5 Changed 8 years ago by
Description:  modified (diff) 

comment:6 Changed 8 years ago by
Status:  needs_review → needs_work 

Work issues:  → merge conflicts 
comment:7 Changed 8 years ago by
Commit:  a74bc0cc7b98f429d69c4bdc842bcb3866ff077e → 2ad6f51d59f2ada78d692e40c3ce96cb99de3d2e 

Branch pushed to git repo; I updated commit sha1. New commits:
2ad6f51  merge conflict resolved

comment:8 Changed 8 years ago by
Commit:  2ad6f51d59f2ada78d692e40c3ce96cb99de3d2e → d4ad7d91a103cea65a9bf1b9e656ea2ef073544e 

Branch pushed to git repo; I updated commit sha1. New commits:
d4ad7d9  conflict resolution corrected

comment:9 Changed 8 years ago by
Status:  needs_work → needs_review 

Work issues:  merge conflicts 
comment:11 Changed 8 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → positive_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
andcopy
attributes.
They are ignored indeed, but it the safe direction: the code acts if both copy
and coerce
are True.
comment:13 Changed 8 years ago by
Status:  positive_review → needs_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 + rowbyrow from top to bottom (so they would + be mostly 0).
and similar changes.
comment:14 Changed 8 years ago by
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 rowbyrow from top to bottom (so they would be mostly 0).
comment:15 Changed 8 years ago by
Commit:  d4ad7d91a103cea65a9bf1b9e656ea2ef073544e → 37bd776aebdcb2f997f4ab2598275a2b2b97c2c2 

Branch pushed to git repo; I updated commit sha1. New commits:
37bd776  doc fixed

comment:16 Changed 8 years ago by
Status:  needs_work → positive_review 

Fixed (and one typo too); thank you!
comment:17 Changed 8 years ago by
Branch:  public/matrix/docinputmatrixclass → 37bd776aebdcb2f997f4ab2598275a2b2b97c2c2 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Merge branch 'public/matrix/docinputmatrixclass' of git://trac.sagemath.org/sage into matrix
also fix doc in matrix_generic_sparse.py