Opened 13 years ago

Closed 13 years ago

#3704 closed defect (fixed)

[with patch, positive review] make diagonal_matrix accept much more general arguments

Reported by: jkantor Owned by: was
Priority: major Milestone: sage-3.3
Component: linear algebra Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

So I think this is a bug

sage: w=vector(RR,[1,2,3])
sage: d=diagonal_matrix(w)
UnboundLocalError: local variable 'v' referenced before assignment

The following fails as well

sage: d=diagonal_matrix(RR,w) 

the only thing that works is

sage: d=diagonal_matrix(RR,list(w))

A stupid but easy fix is to try to turn any argument to diagonal_matrix into a list before bailing out (its in matrix/constructor.py), but there should probably be logic actually expecting vectors and analyzing the parents?

Attachments (5)

trac-3704-diagonal_matrix.patch (4.3 KB) - added by jason 13 years ago.
trac-3704-diagonal_matrix-2.patch (1.3 KB) - added by jason 13 years ago.
trac-3704-diagonal_matrix-3.patch (691 bytes) - added by cremona 13 years ago.
trac-3704-diagonal_matrix-4.patch (816 bytes) - added by cremona 13 years ago.
trac-3704.patch (3.1 KB) - added by rlm 13 years ago.
Independent of the other patches found here.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 13 years ago by jason

Would it be easy as taking the argument to diagonal_matrix and sending it through Sequence?

sage: v=vector(QQ,[1,2,3/4])
sage: v
(1, 2, 3/4)
sage: b=diagonal_matrix(Sequence(v)); b

[  1   0   0]
[  0   2   0]
[  0   0 3/4]
sage: b.parent()
Full MatrixSpace of 3 by 3 dense matrices over Rational Field

comment:2 Changed 13 years ago by jason

  • Priority changed from minor to major
  • Summary changed from diagonal_matrix does not accept vectors to [with patch, needs review] make diagonal_matrix accept much more general arguments

The attached patch is a complete rewrite of diagonal_matrix which now does the following:

  1. If the first argument is a ring, pop it off the argument list
  2. If the next thing can be made into a Sequence, do so and use those elements as the diagonal elements. If the next thing cannot be made into a sequence, then take the remainder of the argument list and use those things as the diagonal elements.
  3. Prepend the ring if one was specified and send everything to the matrix() function.

This changes the syntax slightly; in order to get a matrix of a specific size, you need to pass an nrows and/or ncols option. But it is very nice in that it allows diagonal_matrix(1,2,3) or, as mentioned in this ticket, diagonal_matrix(v) where v is a vector.

All doctests in sage/matrix/* pass and all doctests elsewhere that I found using diagonal_matrix also pass.

comment:3 Changed 13 years ago by jason

  • Type changed from defect to enhancement

comment:4 Changed 13 years ago by jason

  • Type changed from enhancement to defect

I guess it really is a defect, as pointed about above; there is a bug that is fixed.

comment:5 Changed 13 years ago by was

  • Milestone changed from sage-3.0.6 to sage-3.1

comment:6 Changed 13 years ago by cremona

  • Summary changed from [with patch, needs review] make diagonal_matrix accept much more general arguments to [with patch, positve review with mild reservations] make diagonal_matrix accept much more general arguments

I generally like this patch, and it provides useful functionality.

I applied th patch to 3.0.4 with no problem and all tests in sage/matrix pass.

Two things seemed strange to me:

  1. I cannot see a reason for the provided facility for padding with zeros when nrows is given explicitly. I just cannot think of a time when anyone would need this!
  1. The example diagonal_matrix(GF(2),GF(5)) is really crazy. [It returns a diagonal matrix of size 5 and entries 0,1,0,1,0, using a list of the elements of GF(5) coerced into GF(2)!]

This is surely a weird side-effect rather than a useful example. the same result would come from diagonal_matrix(GF(2),range(5)) which is a little more sane.

That last example would not even work if it were not possible to coerce from GF(5) to GF(2). And I really think that should not be possible. Does it still happen in the "new coercion model", I wonder?

comment:7 Changed 13 years ago by robertwb

The patch looks mostly good, but I heartily agree with points (1) and (2). Also, the following gives an undesired result:

sage: diagonal_matrix(x^3+3, x+1)

If a ring is specified, it is converted even if there is no coercion. This allows stuff like

sage: matrix(GF(101), 2, [1, 1/2, 1/3, 1/4])
[ 1 51]
[34 76]

despite there being no coercion QQ -> GF(101).

comment:8 Changed 13 years ago by robertwb

The patch looks mostly good, but I heartily agree with points (1) and (2). Also, the following gives an undesired result:

sage: diagonal_matrix(x^3+3, x+1)

If a ring is specified, it is converted even if there is no coercion. This allows stuff like

sage: matrix(GF(101), 2, [1, 1/2, 1/3, 1/4])
[ 1 51]
[34 76]

despite there being no coercion QQ -> GF(101).

comment:9 Changed 13 years ago by jason

Thanks for the feedback! I'm at a conference now, so I won't have time to address point 2 for a few days. However, to address point 1: the previous version of diagonal_matrix allowed for the rows to be specified and padded with zeros. I was providing an example which showed how to achieve this effect. Basically, I'm also showing that behavior is like the matrix() command (to which I am just blindly passing all keyword arguments, except for some special behavior about the sparse keyword). So: the behavior is a result of the behavior of matrix() and shows how to achieve a former behavior of diagonal_matrix. Whether or not it is actually useful is another issue: if anyone did use this feature, though, it might be nice to have an example of how to do it with the new diagonal_matrix.

I also agree that coercion should play a role, as pointed out in point 2. I'll look at that early next week, hopefully.

Thanks again!

comment:10 Changed 13 years ago by jason

Okay, with the example:

sage: matrix(GF(101), 2, [1, 1/2, 1/3, 1/4])
[ 1 51]
[34 76]

that comes from GF(101)(1/2) giving 51, etc. I just hand it off to the matrix() constructor, which in turn hands the job off to the MatrixSpace? constructor.

As to another point, I get:

sage: diagonal_matrix(x^3+3, x+1)

[x^3 + 3       0]
[      0   x + 1]

so with x being a symbolic variable, things work fine.

However, if we have an iterable object, then things are not so good. In that case, the patch tries to construct a Sequence object from the iterable object, which is where you get your weird results.

I'm changing the patch so that if a list or tuple is passed in, then it tries to construct Sequence object from that list or tuple. Note that this is only for backwards-compatibility, so we can still pass a list into diagonal_matrix and have it return what it used to return.

Changed 13 years ago by jason

comment:11 Changed 13 years ago by jason

  • Summary changed from [with patch, positve review with mild reservations] make diagonal_matrix accept much more general arguments to [with patch, positve review with mild reservations (new patch up which addresses the reservations)] make diagonal_matrix accept much more general arguments

Okay, new patch is up which should take of the issues in my previous post.

comment:12 Changed 13 years ago by cremona

  • Summary changed from [with patch, positve review with mild reservations (new patch up which addresses the reservations)] make diagonal_matrix accept much more general arguments to [with patch, with positive review] make diagonal_matrix accept much more general arguments

I successfully applied the patch to 3.1.alpha0 and all seems to work as advertised. All tests in sage.matrix pass. Publish!

comment:13 Changed 13 years ago by mabshoff

Patches at #2577 seem to do something very close to this patch, so someone ought to sort out if there are any differences that could be resolved into one unified patch.

Cheers,

Michael

comment:14 Changed 13 years ago by jason

I looked at the patch at #2577 and like the patch here better. I can't see any extra functionality in #2577 that I would want to merge to this patch, but I'm open to suggestions.

Changed 13 years ago by jason

comment:15 Changed 13 years ago by jason

I just added a patch, to be applied on top of the first patch, which does two things:

  1. Makes a trivial simplification in the code
  1. Adds a doctest illustrating the sparse keyword.

comment:16 Changed 13 years ago by mabshoff

  • Summary changed from [with patch, with positive review] make diagonal_matrix accept much more general arguments to [with patch, needs review] make diagonal_matrix accept much more general arguments

Hmm, can we sort out this mess, i.e. what to do about #2577? The consensus seems to be to merge theses patches and close #2577?

An final positive review will also be good since Jason did add a second patch.

Cheers,

Michael

Changed 13 years ago by cremona

comment:17 Changed 13 years ago by cremona

  • Summary changed from [with patch, needs review] make diagonal_matrix accept much more general arguments to [with patch, with positive review] make diagonal_matrix accept much more general arguments

Let's agree to kill #2577 since it does nothing which these patches do not do, and these are now a lot better.

I applied both patches to 3.1.2.alpha3 with no problems. All the above examples now work and give sensible answers. They are not all included as doctests though.

I hit a doctest error in sage/matrix/matrix_integer_dense_hnf.py:

sage -t  devel/sage/sage/matrix/matrix_integer_dense_hnf.py **********************************************************************
File "/home/john/sage-3.1.2.alpha3/tmp/matrix_integer_dense_hnf.py", line 132:
    sage: m = diagonal_matrix(ZZ, 68, [2]*66 + [1,1])
Exception raised:
    Traceback (most recent call last):
      File "/home/john/sage-3.1.2.alpha3/local/lib/python2.5/doctest.py", line 1228, in __run
        compileflags, 1) in test.globs
      File "<doctest __main__.example_3[17]>", line 1, in <module>
        m = diagonal_matrix(ZZ, Integer(68), [Integer(2)]*Integer(66) + [Integer(1),Integer(1)])###line 132:
    sage: m = diagonal_matrix(ZZ, 68, [2]*66 + [1,1])
      File "/home/john/sage-3.1.2.alpha3/local/lib/python2.5/site-packages/sage/matrix/constructor.py", line 736, in diagonal_matrix
        return matrix(*args, **kwds)
      File "/home/john/sage-3.1.2.alpha3/local/lib/python2.5/site-packages/sage/matrix/constructor.py", line 524, in matrix
        entries, entry_ring = prepare_dict(args[0])
      File "/home/john/sage-3.1.2.alpha3/local/lib/python2.5/site-packages/sage/matrix/constructor.py", line 619, in prepare_dict
        entries, ring = prepare(X)
      File "/home/john/sage-3.1.2.alpha3/local/lib/python2.5/site-packages/sage/matrix/constructor.py", line 613, in prepare
        raise TypeError, "unable to find a common ring for all elements"
    TypeError: unable to find a common ring for all elements

This is trivial to fix, and I added a trivial patch which fixes it.

Conclusion: kill #2577 and merge this one (all three patches). I assume mabshoff can take on the responsibility of reviewing the final mini-patch!

Changed 13 years ago by cremona

comment:18 Changed 13 years ago by cremona

PS Forgot to add that doctest: 4th patch does that too.

comment:19 Changed 13 years ago by mhansen

Is there a reason that we got rid of this construction?

 	        4. matrix(ring, nrows, diagonal_entries, [sparse=True]): 
	               matrix with given number of rows and flat list of entries  

It should at least be deprecated first.

comment:20 Changed 13 years ago by cremona

There was exactly one doctest which stopped working with that construction missing, and the fix was just to delete the nrows parameter.

I don't have strong feelings about deprecating things like this. I think the point of having nrows in there was so that diagonal_entries could be shorter than nrows and then would be padded out by zeroes. I feel happier requiring the user to provide the whole list of diagonal entries, but if someone wants to put this construction back I don't mind either.

comment:21 Changed 13 years ago by mabshoff

  • Summary changed from [with patch, with positive review] make diagonal_matrix accept much more general arguments to [with patch, needs work] make diagonal_matrix accept much more general arguments

There was some extended discussion on sage-devel and the consensus is that the new non-list constructor will break if more than 255 elements are passed. So this needs work :)

Cheers,

Michael

comment:22 Changed 13 years ago by rlm

  • Summary changed from [with patch, needs work] make diagonal_matrix accept much more general arguments to [with patch, needs review] make diagonal_matrix accept much more general arguments

I'm attaching a new patch which takes a much lower level approach to solving the problem. If a complete rewrite is really necessary, I propose we open a new ticket for it, since this ticket is just about constructing a diagonal matrix from a vector.

Changed 13 years ago by rlm

Independent of the other patches found here.

comment:23 Changed 13 years ago by jason

  • Summary changed from [with patch, needs review] make diagonal_matrix accept much more general arguments to [with patch, positive review] make diagonal_matrix accept much more general arguments

Positive review for rlm's patch. It fixes the problem mentioned in the ticket. The new ticket rlm mentioned is #5110.

comment:24 Changed 13 years ago by jason

(because of #5110, do not delete the other patches on this ticket.)

comment:25 Changed 13 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.3
  • Resolution set to fixed
  • Status changed from new to closed

Merged trac-3704.patch only in Sage 3.3.alpha3.

Cheers,

Michael

Note: See TracTickets for help on using tickets.