Rewrite diagonal matrix constructor #10604

rbeezer · 2011-01-12T18:53:22Z

Diagonal matrix constructor fails when given a tuple, and there is a request to support numpy arrays as input. This seems easiest to accomplish with a re-write and documentation upgrade.

NumPy array request:
http://groups.google.com/group/sage-devel/browse_thread/thread/f0ecd06fcf9efb1b

sage: diagonal_matrix( (1,2,3) )
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)

/home/sage/sage-4.6.1.rc1/devel/sage-main/<ipython console> in <module>()

/home/sage/sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/matrix/constructor.pyc in diagonal_matrix(arg0, arg1, arg2, sparse)
   1271 
   1272     if ring is None:
-> 1273         return matrix(nrows, nrows, w, sparse=sparse)
   1274     else:
   1275         return matrix(ring, nrows, nrows, w, sparse=sparse)

/home/sage/sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/matrix/constructor.pyc in matrix(*args, **kwds)
    577                         ncols = len(args[0]) // nrows
    578                     elif ncols != len(args[0]) // nrows:
--> 579                         raise ValueError, "entries has the wrong length"
    580                 elif len(args[0]) > 0:
    581                     raise ValueError, "entries has the wrong length"

ValueError: entries has the wrong length

Apply:

Depends: #10626

CC: @dandrake

Component: linear algebra

Author: Rob Beezer, Dan Drake

Reviewer: Dan Drake, Rob Beezer

Merged: sage-4.6.2.alpha4

Issue created by migration from https://trac.sagemath.org/ticket/10604

The text was updated successfully, but these errors were encountered:

rbeezer · 2011-01-12T21:56:50Z

comment:3

Attachment: trac_10604-diagonal-matrix-constructor.patch.gz

Patch contains an overhaul of the diagonal_matrix() constructor, fixing at least one bug, and adding support for NumPy arrays as input. Two observations:

Results are now sparse matrices by default. This is a change in behavior, but causes no doctest failures anywhere in the Sage library.
Original doctests for this function will still pass. They are gone now, but were the last thing to be deleted and were fully tested.

dandrake · 2011-01-12T22:20:58Z

Reviewer: Dan Drake

dandrake · 2011-01-12T22:20:58Z

Author: Rob Beezer

dandrake · 2011-01-12T22:20:58Z

comment:4

I read over the code and it looks nice. I love patches that add tons and tons of documentation! I'll run some doctests and make sure this works properly.

dandrake · 2011-01-12T23:18:01Z

comment:5

Hmm, this is weird:

sage: v = numpy.array([1,2,3,100000])
sage: m = diagonal_matrix(v)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/<ipython console> in <module>()

/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/matrix/constructor.pyc in diagonal_matrix(arg0, arg1, arg2, sparse)
   1392         if str(entries.dtype).count('complex')==1:
   1393             ring = CDF
-> 1394         v = [ring(x) for x in entries]
   1395     else:
   1396         raise TypeError('diagonal matrix entries are not a supported type (list, tuple, vector, or NumPy array)')

TypeError: 'NoneType' object is not callable

Your examples seem to have all RDF entries; my example has numpy int64 entries. Do you see what's going on here?

dandrake · 2011-01-12T23:28:21Z

comment:6

I'm also seeing a doctest failure:

drake@sage.math:/scratch/drake/days27/sage-4.6.1.rc1$ ./sage -t  devel/sage-main/sage/rings/number_field/number_field_ideal.p
y
sage -t  "devel/sage-main/sage/rings/number_field/number_field_ideal.py" 
**********************************************************************   
File "/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/devel/sage-main/sage/rings/number_field/number_field_ideal.py", line 1831
:
    sage: units=I.invertible_residues()
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_61[9]>", line 1, in <module>
        units=I.invertible_residues()###line 1831:
    sage: units=I.invertible_residues()
      File "/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/local/lib/python/site-packages/sage/rings/number_field/number_field_ideal.py", line 1846, in invertible_residues
        return self.invertible_residues_mod(subgp_gens=None, reduce=reduce)
      File "/mnt/usb1/scratch/drake/days27/sage-4.6.1.rc1/local/lib/python/site-packages/sage/rings/number_field/number_field_ideal.py", line 1929, in invertible_residues_mod
        A, U, V = M.smith_form()
      File "matrix2.pyx", line 7108, in sage.matrix.matrix2.Matrix.smith_form (sage/matrix/matrix2.c:37793)
      File "matrix2.pyx", line 7269, in sage.matrix.matrix2._smith_diag (sage/matrix/matrix2.c:39676)
      File "integer.pyx", line 5163, in sage.rings.integer.Integer.inverse_mod (sage/rings/integer.c:28740)
      File "integer.pyx", line 367, in sage.rings.integer.as_Integer (sage/rings/integer.c:6052)
      File "integer.pyx", line 680, in sage.rings.integer.Integer.__init__ (sage/rings/integer.c:7312)
    TypeError: unable to coerce <class 'sage.rings.ideal.Ideal_pid'> to an integer

...and a bunch of similar failures in the same file.

rbeezer · 2011-01-13T00:57:00Z

comment:7

Replying to @dandrake:

Thanks, Dan. The int64 makes sense, I think I need to return the NumPy matrices sooner, and coerce them further. The Smith Normal Form certainly should involve diagonal matrices, but I can't quite see where that is coming from (and the failure was not evident in my tests run last night).

I'll get back to it again in a bit.

Rob

rbeezer · 2011-01-13T16:49:33Z

comment:8

Replying to @dandrake:

I'm also seeing a doctest failure:

drake@sage.math:/scratch/drake/days27/sage-4.6.1.rc1$ ./sage -t  devel/sage-main/sage/rings/number_field/number_field_ideal.py

Smith Form seems to be unhappy about getting a sparse matrix:

sage: B=diagonal_matrix(ZZ, [3,4])
sage: B.is_sparse()
True
sage: B.smith_form()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

<snip>

TypeError: unable to coerce <class 'sage.rings.ideal.Ideal_pid'> to an integer

sage: C=B.dense_matrix()
sage: C.smith_form()
(
[ 1  0]  [ 1  1]  [-1 -4]
[ 0 12], [-4 -3], [ 1  3]
)

rbeezer · 2011-01-13T19:14:02Z

comment:9

Sparse integer matrices get sent to the generic Smith form routine (which I suspect is broken, #10625). It fails on an integer matrix. Solution is to direct sparse integer matrices to the dense version and report the generic version.

That change is at #10626, and this ticket will now depend on that patch.

rbeezer · 2011-01-13T22:13:07Z

Attachment: trac_10604-diagonal-matrix-constructor-v2.patch.gz

rbeezer · 2011-01-13T22:15:12Z

comment:10

v2 patch depends on #10626 to avoid the sparse/smith-form problem, and the numpy types are now all automatically handled by the prepare() method for free module elements. Passes tests in sage/matrix and I'll start full tests right now.

rbeezer · 2011-01-13T22:23:51Z

comment:11

This will likely depend on the following sequence:

4.6.1.rc1, #10364, #10537, #10626

rbeezer · 2011-01-13T23:25:20Z

comment:12

Passes all tests in devel/sage with patches described above, so I think this is ready now.

dandrake · 2011-01-20T09:48:14Z

comment:13

Green light from the patchbot...code is good. Positive review.

Release manager: apply #10537 and #10626 first, then only attachment: trac_10604-diagonal-matrix-constructor-v2.patch.

rbeezer · 2011-01-20T19:46:18Z

comment:14

Hi Dan,

Thanks for your help with this and staying with it - much improved based on your testing (and I may be able to excise some NumPy-specific code in the vector constructor based on the prepare() experience here.

I missed seeing the green light. Dang. I've not been paying much attention to the patchbot since it would fail so often on the startup tests, but maybe that is fixed now.

Rob

jdemeyer · 2011-01-26T22:10:56Z

comment:15

There are some doctest failures:

sage -t  "devel/sage/sage/rings/number_field/number_field_ideal.py"
**********************************************************************
File "/usr/local/src/sage-4.6.2.alpha1/devel/sage/sage/rings/number_field/number_field_ideal.py", line 2304:
    sage: I.ideallog(a + 7, [1+a, 2])
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_71[11]>", line 1, in <module>
        I.ideallog(a + Integer(7), [Integer(1)+a, Integer(2)])###line 2304:
    sage: I.ideallog(a + 7, [1+a, 2])
      File "/usr/local/src/sage-4.6.2.alpha1/local/lib/python/site-packages/sage/rings/number_field/number_field_ideal.py", line 2357, in ideallog
        mat = mat.stack( diagonal_matrix(ZZ, invs).augment(zero_matrix(ZZ, len(invs), len(gens))))
      File "matrix_integer_dense.pyx", line 4426, in sage.matrix.matrix_integer_dense.Matrix_integer_dense.stack (sage/matrix/matrix_integer_dense.c:32907)
    TypeError: Cannot convert sage.matrix.matrix_integer_sparse.Matrix_integer_sparse to sage.matrix.matrix_integer_dense.Matrix_integer_dense
**********************************************************************
File "/usr/local/src/sage-4.6.2.alpha1/devel/sage/sage/rings/number_field/number_field_ideal.py", line 2306:
    sage: I.ideallog(a + 7, [2, 1+a])
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_71[12]>", line 1, in <module>
        I.ideallog(a + Integer(7), [Integer(2), Integer(1)+a])###line 2306:
    sage: I.ideallog(a + 7, [2, 1+a])
      File "/usr/local/src/sage-4.6.2.alpha1/local/lib/python/site-packages/sage/rings/number_field/number_field_ideal.py", line 2357, in ideallog
        mat = mat.stack( diagonal_matrix(ZZ, invs).augment(zero_matrix(ZZ, len(invs), len(gens))))
      File "matrix_integer_dense.pyx", line 4426, in sage.matrix.matrix_integer_dense.Matrix_integer_dense.stack (sage/matrix/matrix_integer_dense.c:32907)
    TypeError: Cannot convert sage.matrix.matrix_integer_sparse.Matrix_integer_sparse to sage.matrix.matrix_integer_dense.Matrix_integer_dense
**********************************************************************
File "/usr/local/src/sage-4.6.2.alpha1/devel/sage/sage/rings/number_field/number_field_ideal.py", line 2313:
    sage: J.ideallog(5+2*b, [u, v], check=True)
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_71[17]>", line 1, in <module>
        J.ideallog(Integer(5)+Integer(2)*b, [u, v], check=True)###line 2313:
    sage: J.ideallog(5+2*b, [u, v], check=True)
      File "/usr/local/src/sage-4.6.2.alpha1/local/lib/python/site-packages/sage/rings/number_field/number_field_ideal.py", line 2357, in ideallog
        mat = mat.stack( diagonal_matrix(ZZ, invs).augment(zero_matrix(ZZ, len(invs), len(gens))))
      File "matrix_integer_dense.pyx", line 4426, in sage.matrix.matrix_integer_dense.Matrix_integer_dense.stack (sage/matrix/matrix_integer_dense.c:32907)
    TypeError: Cannot convert sage.matrix.matrix_integer_sparse.Matrix_integer_sparse to sage.matrix.matrix_integer_dense.Matrix_integer_dense
**********************************************************************
File "/usr/local/src/sage-4.6.2.alpha1/devel/sage/sage/rings/number_field/number_field_ideal.py", line 2318:
    sage: I.ideallog(a + 7, [2])
Expected:
    Traceback (most recent call last):
    ...
    ValueError: Given elements do not generate unit group -- they generate a subgroup of index 36
Got:
    Traceback (most recent call last):
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/local/src/sage-4.6.2.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_71[18]>", line 1, in <module>
        I.ideallog(a + Integer(7), [Integer(2)])###line 2318:
    sage: I.ideallog(a + 7, [2])
      File "/usr/local/src/sage-4.6.2.alpha1/local/lib/python/site-packages/sage/rings/number_field/number_field_ideal.py", line 2357, in ideallog
        mat = mat.stack( diagonal_matrix(ZZ, invs).augment(zero_matrix(ZZ, len(invs), len(gens))))
      File "matrix_integer_dense.pyx", line 4426, in sage.matrix.matrix_integer_dense.Matrix_integer_dense.stack (sage/matrix/matrix_integer_dense.c:32907)
    TypeError: Cannot convert sage.matrix.matrix_integer_sparse.Matrix_integer_sparse to sage.matrix.matrix_integer_dense.Matrix_integer_dense
**********************************************************************

dandrake · 2011-01-27T04:26:27Z

comment:16

Replying to @jdemeyer:

There are some doctest failures:

The second-to-last comment says that you need to apply #10626 first.

rbeezer · 2011-01-27T05:30:26Z

comment:17

Dan,

Did you try this against a very recent alpha? (4.6.2.alpha1 or better?). The failures look exactly like what happened on #4492, owing to #10443 that snuck in. (Wrong culprit listed on #4492).

I'm on the road so cannot check immediately, but will soon.

Thanks,
Rob

dandrake · 2011-01-27T06:57:35Z

comment:18

Replying to @rbeezer:

Dan,

Did you try this against a very recent alpha? (4.6.2.alpha1 or better?). The failures look exactly like what happened on #4492, owing to #10443 that snuck in. (Wrong culprit listed on #4492).

I tried it against 4.6.2.alpha2, and I think I spoke too soon above...I applied #10626 and I'm still seeing the failure. So this does need some work, but (like Rob) I won't be able to do it in the next couple days.

dandrake · 2011-01-27T12:45:22Z

comment:19

Replying to @jdemeyer:

There are some doctest failures:

The root of the problem is line 4426 in the stack() method in matrix_integer_dense.pyx:

cdef Matrix_integer_dense A = other

which works poorly when given a sparse matrix.

Interestingly, it works the other way around: you can stack a sparse matrix on top of a dense one. So we need to fix stack() for dense matrices.

dandrake · 2011-01-27T14:18:50Z

fix failing doctest in number_field_ideal.py

dandrake · 2011-01-27T14:44:22Z

comment:20

Attachment: trac_10604_fix_matrix_stack.gz

Can someone who knows the matrix code take a look at my patch? All doctests pass with the two patches here applied to 4.6.2.alpha2.

rbeezer · 2011-01-27T18:17:06Z

comment:22

Replying to @dandrake:

Can someone who knows the matrix code take a look at my patch? All doctests pass with the two patches here applied to 4.6.2.alpha2.

Thanks, Dan. I'll look at this more carefully this evening - should have time then.

The root problem here is my decision to return a diagonal matrix as sparse since that is exposing a variety of inconsistencies. If I had it to do over....

Anyway, I suspect augment() suffers the same affliction. I jazzed up augment() on #10424 with a promise to sync up stack() so maybe I better make good on that.

Thanks for the detective work on this one.

Rob

rbeezer · 2011-01-29T04:37:20Z

Changed author from Rob Beezer to Rob Beezer, Dan Drake

rbeezer · 2011-01-29T04:37:20Z

Changed reviewer from Dan Drake to Dan Drake, Rob Beezer

rbeezer · 2011-01-29T04:37:20Z

comment:23

Replying to @dandrake:

Can someone who knows the matrix code take a look at my patch? All doctests pass with the two patches here applied to 4.6.2.alpha2.

Dan's patch looks good and with it, all tests pass. So I'll flip this back to positive review.

Thanks, Dan.

jdemeyer · 2011-02-07T08:15:35Z

Merged: sage-4.6.2.alpha4

rbeezer mannequin added this to the sage-4.6.2 milestone Jan 12, 2011

rbeezer mannequin added c: linear algebra labels Jan 12, 2011

rbeezer mannequin assigned jasongrout and williamstein Jan 12, 2011

This comment has been minimized.

Sign in to view

rbeezer mannequin added the s: needs review label Jan 12, 2011

dandrake assigned dandrake and unassigned jasongrout and williamstein Jan 12, 2011

rbeezer mannequin added s: needs work and removed s: needs review labels Jan 13, 2011

rbeezer mannequin added s: needs review and removed s: needs work labels Jan 13, 2011

dandrake removed the s: needs review label Jan 20, 2011

dandrake added the s: positive review label Jan 20, 2011

jdemeyer added s: needs work and removed s: positive review labels Jan 26, 2011

dandrake added s: positive review and removed s: needs work labels Jan 27, 2011

dandrake added s: needs work and removed s: positive review labels Jan 27, 2011

dandrake added s: needs review and removed s: needs work labels Jan 27, 2011

This comment has been minimized.

Sign in to view

rbeezer mannequin added s: positive review and removed s: needs review labels Jan 29, 2011

jdemeyer removed the s: positive review label Feb 7, 2011

jdemeyer closed this as completed Feb 7, 2011

rbeezer mannequin mentioned this issue Jan 27, 2011

Route sparse integer matrices to a working Smith form #10626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite diagonal matrix constructor #10604

Rewrite diagonal matrix constructor #10604

rbeezer mannequin commented Jan 12, 2011

This comment has been minimized.

rbeezer mannequin commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

dandrake commented Jan 20, 2011

rbeezer mannequin commented Jan 20, 2011

jdemeyer commented Jan 26, 2011

dandrake commented Jan 27, 2011

rbeezer mannequin commented Jan 27, 2011

dandrake commented Jan 27, 2011

dandrake commented Jan 27, 2011

dandrake commented Jan 27, 2011

dandrake commented Jan 27, 2011

This comment has been minimized.

rbeezer mannequin commented Jan 27, 2011

rbeezer mannequin commented Jan 29, 2011

rbeezer mannequin commented Jan 29, 2011

This comment has been minimized.

rbeezer mannequin commented Jan 29, 2011

jdemeyer commented Feb 7, 2011

Rewrite diagonal matrix constructor #10604

Rewrite diagonal matrix constructor #10604

Comments

rbeezer mannequin commented Jan 12, 2011

This comment has been minimized.

rbeezer mannequin commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

dandrake commented Jan 12, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

rbeezer mannequin commented Jan 13, 2011

dandrake commented Jan 20, 2011

rbeezer mannequin commented Jan 20, 2011

jdemeyer commented Jan 26, 2011

dandrake commented Jan 27, 2011

rbeezer mannequin commented Jan 27, 2011

dandrake commented Jan 27, 2011

dandrake commented Jan 27, 2011

dandrake commented Jan 27, 2011

dandrake commented Jan 27, 2011

This comment has been minimized.

rbeezer mannequin commented Jan 27, 2011

rbeezer mannequin commented Jan 29, 2011

rbeezer mannequin commented Jan 29, 2011

This comment has been minimized.

rbeezer mannequin commented Jan 29, 2011

jdemeyer commented Feb 7, 2011