Opened 8 years ago
Closed 21 months ago
#15835 closed defect (duplicate)
Smith Normal Form Integers Mod 2 TypeError: submatrix() takes exactly 4 positional arguments(2 given)
Reported by:  andrewsilver  Owned by:  

Priority:  minor  Milestone:  sageduplicate/invalid/wontfix 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Reviewers:  Dave Morris  
Report Upstream:  N/A  Work issues:  
Branch:  u/andrewsilver/ticket/15835 (Commits, GitHub, GitLab)  Commit:  feb0831bb267b164837e73ee2801f080b7e15bb6 
Dependencies:  Stopgaps: 
Description (last modified by )
Try the following (Mac OS X Mavericks, Sage 6.11 Master Branch):
A = Matrix(IntegerModRing(2),[[1,0,2],[1,0,1]]) A.smith_form()
You get the error:
> TypeError Traceback (most recent call > last) <ipythoninput623e95c6be019> in <module>() > 1 > A.smith_form() > /Users/.../builds/sage6.1.1/local/lib/python2.7/sitepackages/sage/matrix/matrix2.so > in sage.matrix.matrix2.Matrix.smith_form > (sage/matrix/matrix2.c:60007)() > /Users/.../builds/sage6.1.1/local/lib/python2.7/sitepackages/sage/matrix/matrix_mod2_dense.so > in sage.matrix.matrix_mod2_dense.Matrix_mod2_dense.submatrix > (sage/matrix/matrix_mod2_dense.c:10429)() > TypeError: submatrix() takes exactly 4 positional arguments (2 > given)
This error does not appear for calculations over the integers mod 3
With the advice from https://groups.google.com/forum/#!topic/sagedevel/7F5xUQFcfXE, I went ahead and created what looks like a patch (to me, anyway):
Inside the smith_form() function in matrix2.pyx:
We have: mm = t.submatrix(1,1)
on line 12413.
The sub matrix function takes five parameters (self, starting row, starting col, # sub matrix rows, # sub matrix columns). From observation, the 1,1 is the starting row/col respectively. Somehow the function is unable to determine ncols and nrows when using the integers mod 2, but after some trial and error I discovered a potential patch:
submatrix_rows = t.rows 1 submatrix_cols = t.cols  1 mm = t.submatrix(1,1, submatrix_rows, submatrix,cols)
The above modification gives the same answers as all the examples in the source code. I tested a couple of other things:
Mod 3 Example: A = Matrix(Integers(3),[[1,0,2],[1,0,1],[1,1,1]]) [1 0 0] [1 0 0] [1 0 1] [0 1 0] [2 0 1] [0 1 1] [0 0 1], [1 2 0], [0 0 1]
(same for mod 3 in current master and modified version)
Example mod 2 (not computable in current version of Sage  checked with hand computation  good luck checking it!) 
sage: C = Matrix(IntegerModRing(2),[[1,0,1,0],[1,1,0,0],[0,1,1,0],[1,0,0,1],[0,0,1,1],[0,1,0,1]]) sage: C.smith_form() ( [1 0 0 0] [1 0 0 0 0 0] [0 1 0 0] [1 1 0 0 0 0] [0 0 1 0] [1 0 0 1 0 0] [1 0 1 1] [0 0 0 0] [1 1 1 0 0 0] [0 1 1 1] [0 0 0 0] [1 0 0 1 1 0] [0 0 1 1] [0 0 0 0], [0 1 0 1 0 1], [0 0 0 1] ) sage: t,u,v=C.smith_form() sage: u.inverse() [1 0 0 0 0 0] [1 1 0 0 0 0] [0 1 0 1 0 0] [1 0 1 0 0 0] [0 0 1 0 1 0] [0 1 1 0 0 1]
Change History (18)
comment:1 Changed 8 years ago by
 Branch set to u/andrewsilver/ticket/15835
 Created changed from 02/19/14 04:51:27 to 02/19/14 04:51:27
 Modified changed from 02/19/14 04:51:27 to 02/19/14 04:51:27
comment:2 Changed 8 years ago by
 Commit set to f636d4b3e4d006437edd0da6295ce8d516db6247
comment:3 Changed 8 years ago by
 Status changed from new to needs_review
comment:4 Changed 8 years ago by
 Description modified (diff)
comment:5 Changed 8 years ago by
 Description modified (diff)
comment:6 Changed 8 years ago by
A rather simpler alternative is to delete the submatrix
methods for the Matrix_mod2_dense
and Matrix_mod2e_dense
classes (in the files sage/matrix/matrix_mod2_dense.pyx
and sage/matrix/matrix_mod2e_dense.pyx
, respectively). This would mean that all matrices would use the same submatrix
method. It's completely unclear to me what the extra methods add, and their different syntax has caused the present problem.
I tried commenting them out. It solved the problem addressed in this ticket, and all doctests still passed. A quick check suggested that there was no discernible change in timings.
comment:7 Changed 8 years ago by
Just verified that this really is problem with the matrix mod 2 itself (try creating a matrix mod 2 and submatrixing it in the interpreter, it needs all 4 args to work)  while for a standard nonmod 2 matrix, submatrix by default goes to the end of the matrix starting from the lrows,lcols.
fwclarke's suggestion sounds good to me
comment:8 Changed 8 years ago by
 Commit changed from f636d4b3e4d006437edd0da6295ce8d516db6247 to 0719ede3305cac5db7ecbd2aa41ac1a8f83d945f
comment:9 Changed 8 years ago by
 Commit changed from 0719ede3305cac5db7ecbd2aa41ac1a8f83d945f to aecde667691df1da708464121ad235b279ade368
Branch pushed to git repo; I updated commit sha1. New commits:
aecde66  undid original smith_form fix

comment:10 Changed 8 years ago by
 Branch changed from u/andrewsilver/ticket/15835 to u/fwclarke/ticket/15835
 Modified changed from 02/19/14 20:33:21 to 02/19/14 20:33:21
comment:11 Changed 8 years ago by
 Commit changed from aecde667691df1da708464121ad235b279ade368 to 11477cab5b08e4dc96cef4718fe3e437f50c9d99
An indentation error was created in 9db7f90 when a space was deleted before def __reduce__(self):
I tried to put the space back, but only discovered that there is still a lot I don't understand about using git.
Once this is done, all that remains is to provide a doctest showing that the problem is solved.
New commits:
11477ca  corrected indentation

comment:12 Changed 8 years ago by
 Branch changed from u/fwclarke/ticket/15835 to u/andrewsilver/ticket/15835
 Modified changed from 02/20/14 12:04:39 to 02/20/14 12:04:39
comment:13 followup: ↓ 14 Changed 8 years ago by
 Commit changed from 11477cab5b08e4dc96cef4718fe3e437f50c9d99 to feb0831bb267b164837e73ee2801f080b7e15bb6
I gotchya fwclarke.
For the dockets, is taking the submatrix of an identity matrix (integers mod 2) sufficient?
New commits:
f37a617  fixed indentation in matrix mod 2 dense file

5383e8f  Merge branch 'u/fwclarke/ticket/15835' of git://trac.sagemath.org/sage into ticket/15835

feb0831  resolved merge conflict with other remote branch

comment:14 in reply to: ↑ 13 Changed 8 years ago by
 Status changed from needs_review to needs_work
Replying to andrewsilver:
For the dockets, is taking the submatrix of an identity matrix (integers mod 2) sufficient?
You must have the same spellchecker as me!
Yes something like
sage: A = identity_matrix(GF(2), 6) sage: A.submatrix(3, 2) [0 1 0 0] [0 0 1 0] [0 0 0 1]
would do. And, as there also needs to be a doctest for the case of higher degree finite fields of characteristic 2, perhaps
sage: F8.<a> = GF(8) sage: A = matrix(4, 4, [a^i for i in range(16)]) sage: A.submatrix(2, 1) [ a^2 a + 1 a^2 + a] [a^2 + 1 1 a]
Of course there should be a doctest for smith_form
too.
comment:15 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:16 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:17 Changed 21 months ago by
 Milestone changed from sage6.4 to sageduplicate/invalid/wontfix
 Reviewers set to Dave Morris
 Status changed from needs_work to positive_review
Duplicate of #18761 (merged in 6.8).
comment:18 Changed 21 months ago by
 Resolution set to duplicate
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
corrected t.rows and t.cols to t.nrows and t.ncols respectively