Opened 8 years ago

Closed 17 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: sage-duplicate/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:

Status badges

Description (last modified by jhpalmieri)

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) <ipython-input-6-23e95c6be019> in <module>() ----> 1 
> A.smith_form() 
> /Users/.../builds/sage-6.1.1/local/lib/python2.7/site-packages/sage/matrix/matrix2.so 
>  in sage.matrix.matrix2.Matrix.smith_form 
> (sage/matrix/matrix2.c:60007)() 
> /Users/.../builds/sage-6.1.1/local/lib/python2.7/site-packages/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/sage-devel/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 andrewsilver

  • 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 git

  • Commit set to f636d4b3e4d006437edd0da6295ce8d516db6247

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

f636d4bcorrected t.rows and t.cols to t.nrows and t.ncols respectively

comment:3 Changed 8 years ago by andrewsilver

  • Status changed from new to needs_review

comment:4 Changed 8 years ago by andrewsilver

  • Description modified (diff)

comment:5 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:6 Changed 8 years ago by fwclarke

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 andrewsilver

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 non-mod 2 matrix, submatrix by default goes to the end of the matrix starting from the lrows,lcols.

fwclarke's suggestion sounds good to me

Last edited 8 years ago by andrewsilver (previous) (diff)

comment:8 Changed 8 years ago by git

  • Commit changed from f636d4b3e4d006437edd0da6295ce8d516db6247 to 0719ede3305cac5db7ecbd2aa41ac1a8f83d945f

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

9db7f90removed matrix mod 2 submatrix method
0719ederemoved matrix submatrix method in mod2e dense matrices file. there is already a version for generic matrices that works.

comment:9 Changed 8 years ago by git

  • Commit changed from 0719ede3305cac5db7ecbd2aa41ac1a8f83d945f to aecde667691df1da708464121ad235b279ade368

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

aecde66undid original smith_form fix

comment:10 Changed 8 years ago by fwclarke

  • 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 fwclarke

  • 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:

11477cacorrected indentation

comment:12 Changed 8 years ago by andrewsilver

  • 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 follow-up: Changed 8 years ago by andrewsilver

  • Commit changed from 11477cab5b08e4dc96cef4718fe3e437f50c9d99 to feb0831bb267b164837e73ee2801f080b7e15bb6

I gotchya fwclarke.

For the dockets, is taking the sub-matrix of an identity matrix (integers mod 2) sufficient?


New commits:

f37a617fixed indentation in matrix mod 2 dense file
5383e8fMerge branch 'u/fwclarke/ticket/15835' of git://trac.sagemath.org/sage into ticket/15835
feb0831resolved merge conflict with other remote branch

comment:14 in reply to: ↑ 13 Changed 8 years ago by fwclarke

  • Status changed from needs_review to needs_work

Replying to andrewsilver:

For the dockets, is taking the sub-matrix 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 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:16 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:17 Changed 17 months ago by gh-DaveWitteMorris

  • Milestone changed from sage-6.4 to sage-duplicate/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 17 months ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.