Opened 10 years ago
Closed 10 years ago
#10876 closed enhancement (fixed)
Create elementary matrices
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | sage-4.7.alpha4 | |
Authors: | Rob Beezer | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Patch adds a matrix constructor to build elementary matrices, which correspond to row operations. These matrices are very useful when teaching properties of determinants.
Attachments (4)
Change History (18)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
comment:3 Changed 10 years ago by
Oh, and the additional docs look fine :)
comment:4 Changed 10 years ago by
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
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
Replying to 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.
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!
Changed 10 years ago by
comment:7 Changed 10 years ago by
- 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
- 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!
Changed 10 years ago by
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 10 years ago by
- Status changed from needs_work to needs_review
Replying to kcrisman:
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/3You 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
- 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 turncol1
andcol2
intorow1
androw2
, 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 thata,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
And ref output looks fine. Just about positive review!
Changed 10 years ago by
comment:12 in reply to: ↑ 10 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Replying to kcrisman:
- 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 turncol1
andcol2
intorow1
androw2
, 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 thata,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
- 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
- Merged in set to sage-4.7.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
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':
FORMAT
is hardly ever used in Sage, ratherINPUT
is used, even formatrix?
. 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.
And this:
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.