Opened 10 years ago

Closed 10 years ago

# Create elementary matrices

Reported by: Owned by: rbeezer jason, was minor sage-4.7 linear algebra sage-4.7.alpha4 Rob Beezer Karl-Dieter Crisman N/A

Patch adds a matrix constructor to build elementary matrices, which correspond to row operations. These matrices are very useful when teaching properties of determinants.

### comment:1 Changed 10 years ago by rbeezer

• Authors set to Rob Beezer
• Status changed from new to needs_review

### comment:2 Changed 10 years ago by kcrisman

• Reviewers set to Karl-Dieter Crisman
• Status changed from needs_review to needs_work

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.

Just a few questions, and then a 'needs work':

• 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.
• Super-picky - `FORMAT` is hardly ever used in Sage, rather `INPUT` is used, even for `matrix?`. Any particular reasoning behind this one? I'm asking just in terms of consistency.

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?

Here is an example of what it can lead to.

```sage: elementary_matrix(4,3,3,3)
[1 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
```

And this:

```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]
```

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.

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.

Anyway, none of this takes away from this being a nice addition for LA teaching with Sage.

### comment:3 Changed 10 years ago by kcrisman

Oh, and the additional docs look fine :)

### comment:4 Changed 10 years ago by rbeezer

KDC,

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.

Search `sage/matrix/constructor.py` which has things like `FORMAT` and `CALL FORMAT`. 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!

A reminder about row numbering won't hurt - I agree that this is a place to be careful about that.

Rob

### comment:5 follow-up: ↓ 6 Changed 10 years ago by rbeezer

KDC,

I'm thinking there is no way to have an optional ring, like so many of the other constructors, and consolidate all three matrices into one function.

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.

And which won't allow the same row for the "add a multiple of a row to a row" version.

Rob

### comment:6 in reply to: ↑ 5 Changed 10 years ago by kcrisman

KDC,

I'm thinking there is no way to have an optional ring, like so many of the other constructors, and consolidate all three matrices into one function.

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.

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!

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... ;)

And which won't allow the same row for the "add a multiple of a row to a row" version.

Haha, yes!

### comment:7 Changed 10 years ago by rbeezer

• Status changed from needs_work to needs_review

KDC,

OK, see if you can shoot a hole or two in this one. ;-) Ring is still optional, but rows/columns, and scale factor must be given by keywords. Column operations are implemented by building the equivalent row operation version and then transposing it.

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.

Thanks for the help with this one.

Rob

### comment:8 follow-up: ↓ 9 Changed 10 years ago by kcrisman

• Status changed from needs_review to needs_work

Nice addition of the column stuff/doc.

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)

Holes coming up!

First, illegal/unwanted input.

• This should be caught. Though see my comments below about keywords versus arguments.
```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]
```
• Here is a similar example from your own doctests. Either we require named arguments, or we don't; this is confusing.
```sage: elementary_matrix(QQ, 1, 0, 0)
[1]
```
• This still works, which it probably shouldn't at all:
```sage: elementary_matrix(4,3,3,3)
[1 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
```
• 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.)
```def elementary_matrix(arg0, arg1=None, **kwds):
```
I think this would make checking for illegal cases easier, too.

Then, examples and tests which could be useful.

• 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):
```sage: elementary_matrix(4,row1=3,row2=2,scale=-4)
sage: elementary_matrix(QQ,4,row1=3,row2=2,scale=4/3)
```
• Other possibilities I don't use, but might be fun if they are intended behavior, which I presume they are:
```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)
```
• The following is something people DO use, though, all the time in Gaussian elimination say, and will wonder why it gives an error:
```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
```
You should be able to massage the default ring so that if there IS a scale keyword, its parent chooses the ring:
```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]
```

Iteration 3 will be the bomb!

### comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 10 years ago by rbeezer

• Status changed from needs_work to needs_review

First, illegal/unwanted input.

• This should be caught. Though see my comments below about keywords versus arguments.
```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]
```

Now a doctest, raises an error.

• Here is a similar example from your own doctests. Either we require named arguments, or we don't; this is confusing.

Just forgot these, now fixed.

• 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.)
```def elementary_matrix(arg0, arg1=None, **kwds):
```

Yes, much better.

• 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):
```sage: elementary_matrix(4,row1=3,row2=2,scale=-4)
sage: elementary_matrix(QQ,4,row1=3,row2=2,scale=4/3)
```

Changed one doctest over QQ to a scale factor of 1/2. Did not add a negative.

• Other possibilities I don't use, but might be fun if they are intended behavior, which I presume they are:
```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)
```

Several different rings appear when testing automated ring defaults.

• The following is something people DO use, though, all the time in Gaussian elimination say, and will wonder why it gives an error:
```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
```
You should be able to massage the default ring so that if there IS a scale keyword, its parent chooses the ring:
```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]
```

This is working now, see new doctests.

Iteration 3 will be the bomb!

Yes?

### comment:10 in reply to: ↑ 9 ; follow-up: ↓ 12 Changed 10 years ago by kcrisman

• Description modified (diff)
• Status changed from needs_review to needs_work
```TypeError: scale must be an element of some ring, not junk
```

Nice.

• 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.)
```def elementary_matrix(arg0, arg1=None, **kwds):
```

Yes, much better.

A few programming ideas to make it more tight, though I don't think these are strictly necessary. Take what you want.

• Couldn't the following just be `n<=0`, since you catch that later? Then you don't have to worry about the thing later.
```if n < 0:
raise ValueError('size of elementary matrix must be positive, not {0}'.format(n))
```
• I'd move the checks like
```if row2 is None and scale is None:
```
to right after where you turn `col1` and `col2` into `row1` and `row2`, to improve readability later.
• I think it might be possible to use simultaneous assignment for what comes after
```elif not row2 is None and scale is None:
```
in the same way that
```a,b = 2,3
```

works.

Iteration 3 will be the bomb!

Yes?

Code checks out, passes tests, weird input doesn't slow it down...

One more weird result:

```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]
```

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 :(

Oh, and you didn't capitalize a letter:

```to determine the representation used.  the default is ``False`` which
```

On the plus side, this will have the biggest error test to example ratio ever!

### comment:11 Changed 10 years ago by kcrisman

And ref output looks fine. Just about positive review!

### comment:12 in reply to: ↑ 10 Changed 10 years ago by rbeezer

• Description modified (diff)
• Status changed from needs_work to needs_review

• Couldn't the following just be `n<=0`, since you catch that later? Then you don't have to worry about the thing later.
```if n < 0:
raise ValueError('size of elementary matrix must be positive, not {0}'.format(n))
```

Yes. Changed this, edited error message, edited two doctests to conform.

• I'd move the checks like
```if row2 is None and scale is None:
```
to right after where you turn `col1` and `col2` into `row1` and `row2`, to improve readability later.

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.

• I think it might be possible to use simultaneous assignment for what comes after
```elif not row2 is None and scale is None:
```
in the same way that
```a,b = 2,3
```

Again, I like the four statements laid out like they are, I think the symmetry of it is a bit easier to read.

One more weird result:

```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]
```

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 :(

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.

Oh, and you didn't capitalize a letter:

```to determine the representation used.  the default is ``False`` which
```

Fixed.

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.

Rob

### comment:13 Changed 10 years ago by kcrisman

• Status changed from needs_review to positive_review

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.

For patchbot - apply only trac_10876-elementary-matrices-v4.patch.

### comment:14 Changed 10 years ago by jdemeyer

• Merged in set to sage-4.7.alpha4
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.