Sage: Ticket #3704: [with patch, positive review] make diagonal_matrix accept much more general arguments
https://trac.sagemath.org/ticket/3704
<p>
So I think this is a bug
</p>
<pre class="wiki">sage: w=vector(RR,[1,2,3])
sage: d=diagonal_matrix(w)
UnboundLocalError: local variable 'v' referenced before assignment
</pre><p>
The following fails as well
</p>
<pre class="wiki">sage: d=diagonal_matrix(RR,w)
</pre><p>
the only thing that works is
</p>
<pre class="wiki">sage: d=diagonal_matrix(RR,list(w))
</pre><p>
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?
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/3704
Trac 1.1.6jasonTue, 22 Jul 2008 14:52:58 GMT
https://trac.sagemath.org/ticket/3704#comment:1
https://trac.sagemath.org/ticket/3704#comment:1
<p>
Would it be easy as taking the argument to diagonal_matrix and sending it through Sequence?
</p>
<pre class="wiki">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
</pre>
TicketjasonTue, 22 Jul 2008 16:37:39 GMTpriority, summary changed
https://trac.sagemath.org/ticket/3704#comment:2
https://trac.sagemath.org/ticket/3704#comment:2
<ul>
<li><strong>priority</strong>
changed from <em>minor</em> to <em>major</em>
</li>
<li><strong>summary</strong>
changed from <em>diagonal_matrix does not accept vectors</em> to <em>[with patch, needs review] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
The attached patch is a complete rewrite of diagonal_matrix which now does the following:
</p>
<ol><li>If the first argument is a ring, pop it off the argument list
</li><li>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.
</li><li>Prepend the ring if one was specified and send everything to the matrix() function.
</li></ol><p>
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.
</p>
<p>
All doctests in sage/matrix/* pass and all doctests elsewhere that I found using diagonal_matrix also pass.
</p>
TicketjasonTue, 22 Jul 2008 16:37:50 GMTtype changed
https://trac.sagemath.org/ticket/3704#comment:3
https://trac.sagemath.org/ticket/3704#comment:3
<ul>
<li><strong>type</strong>
changed from <em>defect</em> to <em>enhancement</em>
</li>
</ul>
TicketjasonTue, 22 Jul 2008 16:38:13 GMTtype changed
https://trac.sagemath.org/ticket/3704#comment:4
https://trac.sagemath.org/ticket/3704#comment:4
<ul>
<li><strong>type</strong>
changed from <em>enhancement</em> to <em>defect</em>
</li>
</ul>
<p>
I guess it really is a defect, as pointed about above; there is a bug that is fixed.
</p>
TicketwasFri, 25 Jul 2008 19:45:49 GMTmilestone changed
https://trac.sagemath.org/ticket/3704#comment:5
https://trac.sagemath.org/ticket/3704#comment:5
<ul>
<li><strong>milestone</strong>
changed from <em>sage-3.0.6</em> to <em>sage-3.1</em>
</li>
</ul>
TicketcremonaTue, 29 Jul 2008 01:58:01 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:6
https://trac.sagemath.org/ticket/3704#comment:6
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, positve review with mild reservations] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
I generally like this patch, and it provides useful functionality.
</p>
<p>
I applied th patch to 3.0.4 with no problem and all tests in
sage/matrix pass.
</p>
<p>
Two things seemed strange to me:
</p>
<ol><li>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!
</li></ol><ol start="2"><li>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)!]
</li></ol><blockquote>
<p>
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.
</p>
</blockquote>
<p>
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?
</p>
TicketrobertwbTue, 29 Jul 2008 08:41:19 GMT
https://trac.sagemath.org/ticket/3704#comment:7
https://trac.sagemath.org/ticket/3704#comment:7
<p>
The patch looks mostly good, but I heartily agree with points (1) and (2). Also, the following gives an undesired result:
</p>
<pre class="wiki">sage: diagonal_matrix(x^3+3, x+1)
</pre><p>
If a ring is specified, it is converted even if there is no coercion. This allows stuff like
</p>
<pre class="wiki">sage: matrix(GF(101), 2, [1, 1/2, 1/3, 1/4])
[ 1 51]
[34 76]
</pre><p>
despite there being no coercion QQ -> GF(101).
</p>
TicketrobertwbTue, 29 Jul 2008 08:42:27 GMT
https://trac.sagemath.org/ticket/3704#comment:8
https://trac.sagemath.org/ticket/3704#comment:8
<p>
The patch looks mostly good, but I heartily agree with points (1) and (2). Also, the following gives an undesired result:
</p>
<pre class="wiki">sage: diagonal_matrix(x^3+3, x+1)
</pre><p>
If a ring is specified, it is converted even if there is no coercion. This allows stuff like
</p>
<pre class="wiki">sage: matrix(GF(101), 2, [1, 1/2, 1/3, 1/4])
[ 1 51]
[34 76]
</pre><p>
despite there being no coercion QQ -> GF(101).
</p>
TicketjasonWed, 30 Jul 2008 09:33:11 GMT
https://trac.sagemath.org/ticket/3704#comment:9
https://trac.sagemath.org/ticket/3704#comment:9
<p>
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.
</p>
<p>
I also agree that coercion should play a role, as pointed out in point 2. I'll look at that early next week, hopefully.
</p>
<p>
Thanks again!
</p>
TicketjasonSat, 02 Aug 2008 14:41:45 GMT
https://trac.sagemath.org/ticket/3704#comment:10
https://trac.sagemath.org/ticket/3704#comment:10
<p>
Okay, with the example:
</p>
<pre class="wiki">sage: matrix(GF(101), 2, [1, 1/2, 1/3, 1/4])
[ 1 51]
[34 76]
</pre><p>
that comes from <code>GF(101)(1/2)</code> giving 51, etc. I just hand it off to the matrix() constructor, which in turn hands the job off to the <a class="missing wiki">MatrixSpace?</a> constructor.
</p>
<p>
As to another point, I get:
</p>
<pre class="wiki">sage: diagonal_matrix(x^3+3, x+1)
[x^3 + 3 0]
[ 0 x + 1]
</pre><p>
so with x being a symbolic variable, things work fine.
</p>
<p>
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.
</p>
<p>
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.
</p>
TicketjasonSat, 02 Aug 2008 17:13:09 GMTattachment set
https://trac.sagemath.org/ticket/3704
https://trac.sagemath.org/ticket/3704
<ul>
<li><strong>attachment</strong>
set to <em>trac-3704-diagonal_matrix.patch</em>
</li>
</ul>
TicketjasonSat, 02 Aug 2008 17:18:46 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:11
https://trac.sagemath.org/ticket/3704#comment:11
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, positve review with mild reservations] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, positve review with mild reservations (new patch up which addresses the reservations)] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
Okay, new patch is up which should take of the issues in my previous post.
</p>
TicketcremonaSat, 09 Aug 2008 16:34:56 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:12
https://trac.sagemath.org/ticket/3704#comment:12
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, positve review with mild reservations (new patch up which addresses the reservations)] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, with positive review] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
I successfully applied the patch to 3.1.alpha0 and all seems to work as advertised. All tests in sage.matrix pass. Publish!
</p>
TicketmabshoffMon, 11 Aug 2008 00:43:19 GMT
https://trac.sagemath.org/ticket/3704#comment:13
https://trac.sagemath.org/ticket/3704#comment:13
<p>
Patches at <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a> 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.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketjasonFri, 15 Aug 2008 23:38:40 GMT
https://trac.sagemath.org/ticket/3704#comment:14
https://trac.sagemath.org/ticket/3704#comment:14
<p>
I looked at the patch at <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a> and like the patch here better. I can't see any extra functionality in <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a> that I would want to merge to this patch, but I'm open to suggestions.
</p>
TicketjasonSat, 16 Aug 2008 00:06:22 GMTattachment set
https://trac.sagemath.org/ticket/3704
https://trac.sagemath.org/ticket/3704
<ul>
<li><strong>attachment</strong>
set to <em>trac-3704-diagonal_matrix-2.patch</em>
</li>
</ul>
TicketjasonSat, 16 Aug 2008 00:07:27 GMT
https://trac.sagemath.org/ticket/3704#comment:15
https://trac.sagemath.org/ticket/3704#comment:15
<p>
I just added a patch, to be applied on top of the first patch, which does two things:
</p>
<ol><li>Makes a trivial simplification in the code
</li></ol><ol start="2"><li>Adds a doctest illustrating the sparse keyword.
</li></ol>
TicketmabshoffTue, 02 Sep 2008 11:26:15 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:16
https://trac.sagemath.org/ticket/3704#comment:16
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, with positive review] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, needs review] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
Hmm, can we sort out this mess, i.e. what to do about <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a>? The consensus seems to be to merge theses patches and close <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a>?
</p>
<p>
An final positive review will also be good since Jason did add a second patch.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketcremonaTue, 02 Sep 2008 12:00:11 GMTattachment set
https://trac.sagemath.org/ticket/3704
https://trac.sagemath.org/ticket/3704
<ul>
<li><strong>attachment</strong>
set to <em>trac-3704-diagonal_matrix-3.patch</em>
</li>
</ul>
TicketcremonaTue, 02 Sep 2008 12:01:44 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:17
https://trac.sagemath.org/ticket/3704#comment:17
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, with positive review] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
Let's agree to kill <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a> since it does nothing which these patches do not do, and these are now a lot better.
</p>
<p>
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.
</p>
<p>
I hit a doctest error in sage/matrix/matrix_integer_dense_hnf.py:
</p>
<pre class="wiki">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
</pre><p>
This is trivial to fix, and I added a trivial patch which fixes it.
</p>
<p>
Conclusion: kill <a class="closed ticket" href="https://trac.sagemath.org/ticket/2577" title="enhancement: [with patches; needs work] improving diagonal_matrix and vector ... (closed: fixed)">#2577</a> and merge this one (all three patches). I assume mabshoff can take on the responsibility of reviewing the final mini-patch!
</p>
TicketcremonaTue, 02 Sep 2008 12:06:04 GMTattachment set
https://trac.sagemath.org/ticket/3704
https://trac.sagemath.org/ticket/3704
<ul>
<li><strong>attachment</strong>
set to <em>trac-3704-diagonal_matrix-4.patch</em>
</li>
</ul>
TicketcremonaTue, 02 Sep 2008 12:06:38 GMT
https://trac.sagemath.org/ticket/3704#comment:18
https://trac.sagemath.org/ticket/3704#comment:18
<p>
PS Forgot to add that doctest: 4th patch does that too.
</p>
TicketmhansenTue, 02 Sep 2008 23:28:50 GMT
https://trac.sagemath.org/ticket/3704#comment:19
https://trac.sagemath.org/ticket/3704#comment:19
<p>
Is there a reason that we got rid of this construction?
</p>
<pre class="wiki"> 4. matrix(ring, nrows, diagonal_entries, [sparse=True]):
matrix with given number of rows and flat list of entries
</pre><p>
It should at least be deprecated first.
</p>
TicketcremonaWed, 03 Sep 2008 08:29:14 GMT
https://trac.sagemath.org/ticket/3704#comment:20
https://trac.sagemath.org/ticket/3704#comment:20
<p>
There was exactly one doctest which stopped working with that construction missing, and the fix was just to delete the nrows parameter.
</p>
<p>
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.
</p>
TicketmabshoffFri, 19 Sep 2008 03:30:05 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:21
https://trac.sagemath.org/ticket/3704#comment:21
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, with positive review] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, needs work] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
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 :)
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketrlmFri, 23 Jan 2009 12:40:18 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:22
https://trac.sagemath.org/ticket/3704#comment:22
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, needs review] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
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.
</p>
TicketrlmFri, 23 Jan 2009 12:41:09 GMTattachment set
https://trac.sagemath.org/ticket/3704
https://trac.sagemath.org/ticket/3704
<ul>
<li><strong>attachment</strong>
set to <em>trac-3704.patch</em>
</li>
</ul>
<p>
Independent of the other patches found here.
</p>
TicketjasonTue, 27 Jan 2009 03:55:24 GMTsummary changed
https://trac.sagemath.org/ticket/3704#comment:23
https://trac.sagemath.org/ticket/3704#comment:23
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] make diagonal_matrix accept much more general arguments</em> to <em>[with patch, positive review] make diagonal_matrix accept much more general arguments</em>
</li>
</ul>
<p>
Positive review for rlm's patch. It fixes the problem mentioned in the ticket. The new ticket rlm mentioned is <a class="closed ticket" href="https://trac.sagemath.org/ticket/5110" title="enhancement: rewrite diagonal_matrix to be more general (closed: duplicate)">#5110</a>.
</p>
TicketjasonTue, 27 Jan 2009 03:55:56 GMT
https://trac.sagemath.org/ticket/3704#comment:24
https://trac.sagemath.org/ticket/3704#comment:24
<p>
(because of <a class="closed ticket" href="https://trac.sagemath.org/ticket/5110" title="enhancement: rewrite diagonal_matrix to be more general (closed: duplicate)">#5110</a>, do not delete the other patches on this ticket.)
</p>
TicketmabshoffWed, 28 Jan 2009 14:12:34 GMTstatus, milestone changed; resolution set
https://trac.sagemath.org/ticket/3704#comment:25
https://trac.sagemath.org/ticket/3704#comment:25
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-3.4.1</em> to <em>sage-3.3</em>
</li>
</ul>
<p>
Merged trac-3704.patch only in Sage 3.3.alpha3.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
Ticket