Opened 10 years ago

Closed 3 years ago

#12887 closed defect (worksforme)

Optional args in Matrix_Mod submatrix method

Reported by: dscharles Owned by: jason, was
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: linear algebra Keywords: matrix, finite field, submatrix
Cc: Merged in:
Authors: David Sanchez Charles Reviewers: Martin Albrecht, Simon Brandhorst
Report Upstream: N/A Work issues: add examples to doctests
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by dscharles)

When computing the smith form of a matrix (over a finite field) it promts an error

sage: AA = matrix(Zmod(2), [[1,1],[1,0],[0,1]])
sage: AA.smith_form()

It promts

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "_sage_input_100.py", line 10, in <module>
    exec compile(u'open("___code___.py","w").write("# -*- coding: utf-8 -*-\\n" + _support_.preparse_worksheet_cell(base64.b64decode("QUEuc21pdGhfZm9ybSgp"),globals())+"\\n"); execfile(os.path.abspath("___code___.py"))
  File "", line 1, in <module>
   
  File "/tmp/tmpXWhShB/___code___.py", line 2, in <module>
    exec compile(u'AA.smith_form()
  File "", line 1, in <module>
   
  File "matrix2.pyx", line 10512, in sage.matrix.matrix2.Matrix.smith_form (sage/matrix/matrix2.c:49285)
  File "matrix_mod2_dense.pyx", line 1666, in sage.matrix.matrix_mod2_dense.Matrix_mod2_dense.submatrix (sage/matrix/matrix_mod2_dense.c:9696)
TypeError: submatrix() takes exactly 4 positional arguments (2 given)

It seems that the "Matrix_mod" submatrix method doesn't have "ncols" and "nrows" as optional parameters (like the base Matrix class).

I used Sage 4.8.0 in an Ubuntu 12.04 (32 bits).

sage-support discussion: https://groups.google.com/forum/?fromgroups#!topic/sage-support/TssHVf3oUeM

Attachments (3)

trac_12887_submatrix_over_finite_field.patch (1.9 KB) - added by dscharles 10 years ago.
trac_12887_changed_output.patch (1.1 KB) - added by dscharles 10 years ago.
It also required some output changes.
trac_12887_Matrix_mod_submatrix_fix.patch (2.5 KB) - added by dscharles 10 years ago.
Merged patch

Download all attachments as: .zip

Change History (20)

Changed 10 years ago by dscharles

comment:1 Changed 10 years ago by dscharles

I attached a patch. I hope it's well done (It's my first one...)

comment:2 Changed 10 years ago by dscharles

  • Description modified (diff)

comment:3 Changed 10 years ago by malb

  • Authors set to David Sanchez Charles
  • Reviewers set to Martin Albrecht
  • Status changed from new to needs_review

Patch looks good to me.

PS: Can you add yourself to http://trac.sagemath.org/sage_trac/wiki please.

comment:4 Changed 10 years ago by malb

  • Status changed from needs_review to positive_review

Changed 10 years ago by dscharles

It also required some output changes.

comment:5 Changed 10 years ago by dscharles

I'm sorry I didn't realized before, but it also was necessary to change the output in order to work properly. (Should I open another ticket?)

Before the last patch:

sage: A = matrix([[1,1],[2,2],[3,3]])
sage: B = matrix(Zmod(2), [[1,1],[2,2],[3,3]])
sage: A.submatrix(2,2)
[]
sage: A.submatrix(2,2).ncols()
0
sage: A.submatrix(2,2).nrows()
1
sage: B
[]
sage: B.submatrix(2,2).ncols()
0
sage: B.submatrix(2,2).nrows()
0

After the patch

sage: B.submatrix(2,2)
[]
sage: B.submatrix(2,2).ncols()
0
sage: B.submatrix(2,2).nrows()
1

With that done, the smith_form method worked with some (easy) examples.

comment:6 Changed 10 years ago by malb

  • Status changed from positive_review to needs_work

Looks good, can you merge the two patches? Also, if you change stuff like that you should switch from positive review to needs review below.

Changed 10 years ago by dscharles

Merged patch

comment:7 Changed 10 years ago by dscharles

  • Status changed from needs_work to needs_review

Merged patch uploaded.

comment:8 Changed 10 years ago by malb

Looks good, but could you also add those examples from this ticket to the doctests to test that your code produces the desired outcome. Sorry for only mentioning that now!

comment:9 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:10 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:11 Changed 8 years ago by rws

  • Status changed from needs_review to needs_work
  • Work issues set to add examples to doctests

comment:12 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

Let us close this old ticket. Works now:

sage: AA = matrix(Zmod(2), [[1,1],[1,0],[0,1]])
sage: AA.smith_form()
(
[1 0]  [1 0 0]       
[0 1]  [1 1 0]  [1 1]
[0 0], [1 1 1], [0 1]
)

comment:15 Changed 4 years ago by sbrandhorst

  • Status changed from needs_review to positive_review

comment:16 Changed 4 years ago by sbrandhorst

  • Reviewers changed from Martin Albrecht to Martin Albrecht, Simon Brandhorst

comment:17 Changed 3 years ago by embray

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