Opened 8 years ago

Closed 2 years ago

# Smith Normal Form Integers Mod 2 TypeError: submatrix() takes exactly 4 positional arguments(2 given)

Reported by: Owned by: andrewsilver minor sage-duplicate/invalid/wontfix linear algebra Dave Morris N/A u/andrewsilver/ticket/15835 feb0831bb267b164837e73ee2801f080b7e15bb6

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]
```

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

 ​f636d4b `corrected 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:

 ​9db7f90 `removed matrix mod 2 submatrix method` ​0719ede `removed 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

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

 ​aecde66 `undid 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

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 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: ↓ 14 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:

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

• Status changed from needs_review to needs_work

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 8 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:16 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:17 Changed 2 years 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 2 years ago by chapoton

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