Sage: Ticket #10876: Create elementary matrices
https://trac.sagemath.org/ticket/10876
<p>
Patch adds a matrix constructor to build elementary matrices, which correspond to row operations. These matrices are very useful when teaching properties of determinants.
</p>
<hr />
<p>
Apply <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10876/trac_10876-elementary-matrices-v4.patch" title="Attachment 'trac_10876-elementary-matrices-v4.patch' in Ticket #10876">trac_10876-elementary-matrices-v4.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10876/trac_10876-elementary-matrices-v4.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10876
Trac 1.1.6rbeezerFri, 04 Mar 2011 23:38:30 GMTattachment set
https://trac.sagemath.org/ticket/10876
https://trac.sagemath.org/ticket/10876
<ul>
<li><strong>attachment</strong>
set to <em>trac_10876-elementary-matrices.patch</em>
</li>
</ul>
TicketrbeezerFri, 04 Mar 2011 23:39:03 GMTstatus changed; author set
https://trac.sagemath.org/ticket/10876#comment:1
https://trac.sagemath.org/ticket/10876#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Rob Beezer</em>
</li>
</ul>
TicketkcrismanSun, 13 Mar 2011 03:40:29 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/10876#comment:2
https://trac.sagemath.org/ticket/10876#comment:2
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>Karl-Dieter Crisman</em>
</li>
</ul>
<p>
This looks really nice, Rob - comprehensive, useful, well-organized and tested. I like your using the Python 3 string formatting, which I have yet to learn - didn't realize it was already supported.
</p>
<p>
Just a few questions, and then a 'needs work':
</p>
<ul><li>Think we need to remind people that the rows are numbered from 0 to n-1? Up to your discretion; if one actually reads and understands the doc, it's clear, but just asking since we sometimes like to remind of things like this that are not standard math notation.
</li><li>Super-picky - <code>FORMAT</code> is hardly ever used in Sage, rather <code>INPUT</code> is used, even for <code>matrix?</code>. Any particular reasoning behind this one? I'm asking just in terms of consistency.
</li></ul><p>
I fear that the notion that the ring is optional will lead to incorrect use of that without keywords. Can you think of any other way to word the first few things?
</p>
<p>
Here is an example of what it can lead to.
</p>
<pre class="wiki">sage: elementary_matrix(4,3,3,3)
[1 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
</pre><p>
And this:
</p>
<pre class="wiki">sage: elementary_matrix(4,3,scale=4)
[4 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
sage: elementary_matrix(4,scale=4)
[4 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
</pre><p>
I don't think that either of these are 'allowed', but the second of these in particular is very tempting, with n=4, row 3, scale by 4.
</p>
<p>
Because of the audience of this patch, I think it is really important to make sure we catch things like this, esp. plausible use cases. It would be nice to not need the ring at all, but I understand that for consistency with other matrix invocations this needs to be the first (optional) argument.
</p>
<p>
Anyway, none of this takes away from this being a nice addition for LA teaching with Sage.
</p>
TicketkcrismanSun, 13 Mar 2011 03:42:37 GMT
https://trac.sagemath.org/ticket/10876#comment:3
https://trac.sagemath.org/ticket/10876#comment:3
<p>
Oh, and the additional docs look fine :)
</p>
TicketrbeezerSun, 13 Mar 2011 05:05:33 GMT
https://trac.sagemath.org/ticket/10876#comment:4
https://trac.sagemath.org/ticket/10876#comment:4
<p>
KDC,
</p>
<p>
Thanks for the testing. I was trying to figure out how to consolidate all three elementary matrices into one function - the problem is when you want to scale a row by an integer scalar, how can you distinguish that from swapping two rows indexed by integers? Maybe I was being too clever, I'll have to study your examples.
</p>
<p>
Search <code>sage/matrix/constructor.py</code> which has things like <code>FORMAT</code> and <code>CALL FORMAT</code>. I have edited many of them recently, but they were there before I got there. These constructors are tricky with the optional ring and then various items that get inferred, or options, or... I don't think it is bad to have a concise summary of what will work right up front - it certainly makes coding them easier!
</p>
<p>
A reminder about row numbering won't hurt - I agree that this is a place to be careful about that.
</p>
<p>
Rob
</p>
TicketrbeezerSun, 13 Mar 2011 23:46:43 GMT
https://trac.sagemath.org/ticket/10876#comment:5
https://trac.sagemath.org/ticket/10876#comment:5
<p>
KDC,
</p>
<p>
I'm thinking there is no way to have an optional ring, like so many of the other constructors, <em>and</em> consolidate all three matrices into one function.
</p>
<p>
I think adding just one function to the global namespace is preferable to the optional ring. But if you have any great ideas about how to accomplish both, let me know. Otherwise I'll make a new version that requires a ring.
</p>
<p>
And which won't allow the same row for the "add a multiple of a row to a row" version.
</p>
<p>
Rob
</p>
TicketkcrismanMon, 14 Mar 2011 12:40:54 GMT
https://trac.sagemath.org/ticket/10876#comment:6
https://trac.sagemath.org/ticket/10876#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10876#comment:5" title="Comment 5">rbeezer</a>:
</p>
<blockquote class="citation">
<p>
KDC,
</p>
<p>
I'm thinking there is no way to have an optional ring, like so many of the other constructors, <em>and</em> consolidate all three matrices into one function.
</p>
</blockquote>
<p>
Unless you make the arguments mandatory (I guess making them keywords). Which, for a pedagogical function, is actually not so bad. I think that would be preferable to having instructors of LA who are not so knowledgeable about rings, or don't want to bother students with them, being forced to use the ring.
</p>
<p>
I usually like flexibility, but keeping these all in one function seems good, and asking for 'row1' and 'scale' seems very appropriate if you're learning what the elementary matrices are in the first place. I really doubt any heavy user is going to be using this function instead of writing a small script to generate their own (possible sparse) matrices!
</p>
<p>
Also, if you were to do this, I figure that in the case of scaling a single row, one could allow the keyword 'row' instead of 'rows'. Any interest in also providing column elementary matrices? I guess one could just multiply on the right... ;)
</p>
<blockquote class="citation">
<p>
And which won't allow the same row for the "add a multiple of a row to a row" version.
</p>
</blockquote>
<p>
Haha, yes!
</p>
TicketrbeezerTue, 15 Mar 2011 00:41:13 GMTattachment set
https://trac.sagemath.org/ticket/10876
https://trac.sagemath.org/ticket/10876
<ul>
<li><strong>attachment</strong>
set to <em>trac_10876-elementary-matrices-v2.patch</em>
</li>
</ul>
TicketrbeezerTue, 15 Mar 2011 00:43:35 GMTstatus changed
https://trac.sagemath.org/ticket/10876#comment:7
https://trac.sagemath.org/ticket/10876#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
KDC,
</p>
<p>
OK, see if you can shoot a hole or two in this one. ;-) Ring is still optional, but rows/columns, and scale factor <em>must</em> be given by keywords. Column operations are implemented by building the equivalent row operation version and then transposing it.
</p>
<p>
Docstring leans towards rows, but I think there is enough info on the column variants. Two small reminders about 0-based indexing. New doctest showing dense implementation as default, but now has sparse keyword added.
</p>
<p>
Thanks for the help with this one.
</p>
<p>
Rob
</p>
TicketkcrismanWed, 16 Mar 2011 12:34:13 GMTstatus changed
https://trac.sagemath.org/ticket/10876#comment:8
https://trac.sagemath.org/ticket/10876#comment:8
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Nice addition of the column stuff/doc.
</p>
<p>
This functionality would be especially valuable as an interact - you choose the elementary matrix type, the row1, row2, scale, and it creates the matrix - or, better, changes the unit circle or a pair of vectors *based* on your elementary matrix. If only we could get interact controls to depend on other interact controls... (yes, there is a ticket for this)
</p>
<p>
Holes coming up!
</p>
<hr />
<p>
First, illegal/unwanted input.
</p>
<ul><li>This should be caught. Though see my comments below about keywords versus arguments.
<pre class="wiki">sage: E = elementary_matrix(ZZ, 5, row1=3, col2=3, scale=12)
sage: E
[ 1 0 0 0 0]
[ 0 1 0 0 0]
[ 0 0 1 0 0]
[ 0 0 0 12 0]
[ 0 0 0 0 1]
</pre></li></ul><ul><li>Here is a similar example from your own doctests. Either we require named arguments, or we don't; this is confusing.
<pre class="wiki">sage: elementary_matrix(QQ, 1, 0, 0)
[1]
</pre></li><li>This still works, which it probably shouldn't at all:
<pre class="wiki">sage: elementary_matrix(4,3,3,3)
[1 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
</pre></li><li>To deal with all this in general, couldn't you do something like the following, and then look for for the specific keywords row1, sparse, etc? (And raise an error if any others appear in the dictionary.)
<pre class="wiki">def elementary_matrix(arg0, arg1=None, **kwds):
</pre>I think this would make checking for illegal cases easier, too.
</li></ul><hr />
<p>
Then, examples and tests which could be useful.
</p>
<ul><li>An example with a negative scale would be nice, though certainly not necessary. Similarly with a rational scale, so that people know it's ok to do so (the doctests scare one away from it):
<pre class="wiki">sage: elementary_matrix(4,row1=3,row2=2,scale=-4)
sage: elementary_matrix(QQ,4,row1=3,row2=2,scale=4/3)
</pre></li><li>Other possibilities I don't use, but might be fun if they are intended behavior, which I presume they are:
<pre class="wiki">sage: elementary_matrix(SR,4,row1=3,row2=2,scale=sqrt(3))
sage: elementary_matrix(SR,4,row1=3,row2=2,scale=i)
sage: elementary_matrix(CC,4,row1=3,row2=2,scale=i)
</pre></li><li>The following is something people DO use, though, all the time in Gaussian elimination say, and will wonder why it gives an error:
<pre class="wiki">sage: sage: elementary_matrix(4,row1=3,scale=4/3)
---------------------------------------------------------------------------
TypeError: scale parameter of elementary matrix must an element of
Integer Ring, not 4/3
</pre>You should be able to massage the default ring so that if there IS a scale keyword, its parent chooses the ring:
<pre class="wiki">sage: elementary_matrix(parent(4/3),4,row1=3,scale=4/3)
[ 1 0 0 0]
[ 0 1 0 0]
[ 0 0 1 0]
[ 0 0 0 4/3]
</pre></li></ul><hr />
<p>
Iteration 3 will be the bomb!
</p>
TicketrbeezerThu, 17 Mar 2011 05:02:20 GMTattachment set
https://trac.sagemath.org/ticket/10876
https://trac.sagemath.org/ticket/10876
<ul>
<li><strong>attachment</strong>
set to <em>trac_10876-elementary-matrices-v3.patch</em>
</li>
</ul>
TicketrbeezerThu, 17 Mar 2011 05:03:50 GMTstatus changed
https://trac.sagemath.org/ticket/10876#comment:9
https://trac.sagemath.org/ticket/10876#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10876#comment:8" title="Comment 8">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
First, illegal/unwanted input.
</p>
<ul><li>This should be caught. Though see my comments below about keywords versus arguments.
<pre class="wiki">sage: E = elementary_matrix(ZZ, 5, row1=3, col2=3, scale=12)
sage: E
[ 1 0 0 0 0]
[ 0 1 0 0 0]
[ 0 0 1 0 0]
[ 0 0 0 12 0]
[ 0 0 0 0 1]
</pre></li></ul></blockquote>
<p>
Now a doctest, raises an error.
</p>
<blockquote class="citation">
<ul><li>Here is a similar example from your own doctests. Either we require named arguments, or we don't; this is confusing.
</li></ul></blockquote>
<p>
Just forgot these, now fixed.
</p>
<blockquote class="citation">
<ul><li>To deal with all this in general, couldn't you do something like the following, and then look for for the specific keywords row1, sparse, etc? (And raise an error if any others appear in the dictionary.)
<pre class="wiki">def elementary_matrix(arg0, arg1=None, **kwds):
</pre></li></ul></blockquote>
<p>
Yes, much better.
</p>
<blockquote class="citation">
<ul><li>An example with a negative scale would be nice, though certainly not necessary. Similarly with a rational scale, so that people know it's ok to do so (the doctests scare one away from it):
<pre class="wiki">sage: elementary_matrix(4,row1=3,row2=2,scale=-4)
sage: elementary_matrix(QQ,4,row1=3,row2=2,scale=4/3)
</pre></li></ul></blockquote>
<p>
Changed one doctest over QQ to a scale factor of 1/2. Did not add a negative.
</p>
<blockquote class="citation">
<ul><li>Other possibilities I don't use, but might be fun if they are intended behavior, which I presume they are:
<pre class="wiki">sage: elementary_matrix(SR,4,row1=3,row2=2,scale=sqrt(3))
sage: elementary_matrix(SR,4,row1=3,row2=2,scale=i)
sage: elementary_matrix(CC,4,row1=3,row2=2,scale=i)
</pre></li></ul></blockquote>
<p>
Several different rings appear when testing automated ring defaults.
</p>
<blockquote class="citation">
<ul><li>The following is something people DO use, though, all the time in Gaussian elimination say, and will wonder why it gives an error:
<pre class="wiki">sage: sage: elementary_matrix(4,row1=3,scale=4/3)
---------------------------------------------------------------------------
TypeError: scale parameter of elementary matrix must an element of
Integer Ring, not 4/3
</pre>You should be able to massage the default ring so that if there IS a scale keyword, its parent chooses the ring:
<pre class="wiki">sage: elementary_matrix(parent(4/3),4,row1=3,scale=4/3)
[ 1 0 0 0]
[ 0 1 0 0]
[ 0 0 1 0]
[ 0 0 0 4/3]
</pre></li></ul></blockquote>
<p>
This is working now, see new doctests.
</p>
<blockquote class="citation">
<p>
Iteration 3 will be the bomb!
</p>
</blockquote>
<p>
Yes?
</p>
TicketkcrismanThu, 17 Mar 2011 15:36:27 GMTstatus, description changed
https://trac.sagemath.org/ticket/10876#comment:10
https://trac.sagemath.org/ticket/10876#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10876?action=diff&version=10">diff</a>)
</li>
</ul>
<pre class="wiki">TypeError: scale must be an element of some ring, not junk
</pre><p>
Nice.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>To deal with all this in general, couldn't you do something like the following, and then look for for the specific keywords row1, sparse, etc? (And raise an error if any others appear in the dictionary.)
<pre class="wiki">def elementary_matrix(arg0, arg1=None, **kwds):
</pre></li></ul></blockquote>
<p>
Yes, much better.
</p>
</blockquote>
<p>
A few programming ideas to make it more tight, though I don't think these are strictly necessary. Take what you want.
</p>
<ul><li>Couldn't the following just be <code>n<=0</code>, since you catch that later? Then you don't have to worry about the thing later.
<pre class="wiki">if n < 0:
raise ValueError('size of elementary matrix must be positive, not {0}'.format(n))
</pre></li><li>I'd move the checks like
<pre class="wiki">if row2 is None and scale is None:
</pre>to right after where you turn <code>col1</code> and <code>col2</code> into <code>row1</code> and <code>row2</code>, to improve readability later.
</li><li>I think it might be possible to use simultaneous assignment for what comes after
<pre class="wiki">elif not row2 is None and scale is None:
</pre>in the same way that
<pre class="wiki">a,b = 2,3
</pre></li></ul><p>
works.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Iteration 3 will be the bomb!
</p>
</blockquote>
<p>
Yes?
</p>
</blockquote>
<p>
Code checks out, passes tests, weird input doesn't slow it down...
</p>
<p>
One more weird result:
</p>
<pre class="wiki">sage: elementary_matrix(4,2,row1=1,row2=3)
[1 0 0 0]
[0 0 0 1]
[0 0 1 0]
[0 1 0 0]
</pre><p>
If the first argument isn't a ring, you just automatically make the ring the integer ring, the size the first argument, and ignore the second argument. Probably this should be caught. Needs work :(
</p>
<p>
Oh, and you didn't capitalize a letter:
</p>
<pre class="wiki">to determine the representation used. the default is ``False`` which
</pre><p>
On the plus side, this will have the biggest error test to example ratio ever!
</p>
TicketkcrismanThu, 17 Mar 2011 15:38:41 GMT
https://trac.sagemath.org/ticket/10876#comment:11
https://trac.sagemath.org/ticket/10876#comment:11
<p>
And ref output looks fine. Just about positive review!
</p>
TicketrbeezerFri, 18 Mar 2011 05:24:27 GMTattachment set
https://trac.sagemath.org/ticket/10876
https://trac.sagemath.org/ticket/10876
<ul>
<li><strong>attachment</strong>
set to <em>trac_10876-elementary-matrices-v4.patch</em>
</li>
</ul>
TicketrbeezerFri, 18 Mar 2011 05:35:10 GMTstatus, description changed
https://trac.sagemath.org/ticket/10876#comment:12
https://trac.sagemath.org/ticket/10876#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10876?action=diff&version=12">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10876#comment:10" title="Comment 10">kcrisman</a>:
</p>
<blockquote class="citation">
<ul><li>Couldn't the following just be <code>n<=0</code>, since you catch that later? Then you don't have to worry about the thing later.
<pre class="wiki">if n < 0:
raise ValueError('size of elementary matrix must be positive, not {0}'.format(n))
</pre></li></ul></blockquote>
<p>
Yes. Changed this, edited error message, edited two doctests to conform.
</p>
<blockquote class="citation">
<ul><li>I'd move the checks like
<pre class="wiki">if row2 is None and scale is None:
</pre>to right after where you turn <code>col1</code> and <code>col2</code> into <code>row1</code> and <code>row2</code>, to improve readability later.
</li></ul></blockquote>
<p>
I like these where they are. Lots of error-checking above to get right inputs. Then the analysis of what we have makes the decision on which of the three elementary matrix types to create. This is how we avoid creating three functions in the global namespace. So I'd like to have it divorced from the other stuff, and split off as the next step.
</p>
<blockquote class="citation">
<ul><li>I think it might be possible to use simultaneous assignment for what comes after
<pre class="wiki">elif not row2 is None and scale is None:
</pre>in the same way that
<pre class="wiki">a,b = 2,3
</pre></li></ul></blockquote>
<p>
Again, I like the four statements laid out like they are, I think the symmetry of it is a bit easier to read.
</p>
<blockquote class="citation">
<p>
One more weird result:
</p>
<pre class="wiki">sage: elementary_matrix(4,2,row1=1,row2=3)
[1 0 0 0]
[0 0 0 1]
[0 0 1 0]
[0 1 0 0]
</pre><p>
If the first argument isn't a ring, you just automatically make the ring the integer ring, the size the first argument, and ignore the second argument. Probably this should be caught. Needs work :(
</p>
</blockquote>
<p>
Fixed with new check as very first two lines of routine. New doctest (first of error checks) will exercise it. Your example is caught as well.
</p>
<blockquote class="citation">
<p>
Oh, and you didn't capitalize a letter:
</p>
<pre class="wiki">to determine the representation used. the default is ``False`` which
</pre></blockquote>
<p>
Fixed.
</p>
<p>
Three for five. Hope that does it. Highest ratio of error-checks to code ever, and a v4 patch is a new personal best. Thanks for your help - this is much better than the original conception.
</p>
<p>
Rob
</p>
TicketkcrismanFri, 18 Mar 2011 14:40:54 GMTstatus changed
https://trac.sagemath.org/ticket/10876#comment:13
https://trac.sagemath.org/ticket/10876#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Okay, that's all fine. Passes tests, fixed that example, and I really don't want to ache more about this. As they say, there are bugs in all software, but there sure aren't a lot more in this, unless we both got the math itself wrong! Hopefully not. Positive review.
</p>
<hr />
<p>
For patchbot - apply only <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10876/trac_10876-elementary-matrices-v4.patch" title="Attachment 'trac_10876-elementary-matrices-v4.patch' in Ticket #10876">trac_10876-elementary-matrices-v4.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10876/trac_10876-elementary-matrices-v4.patch" title="Download"></a>.
</p>
TicketjdemeyerTue, 05 Apr 2011 12:00:44 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10876#comment:14
https://trac.sagemath.org/ticket/10876#comment:14
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.alpha4</em>
</li>
</ul>
Ticket