Changes between Version 10 and Version 12 of Ticket #10876

03/18/11 05:35:10 (10 years ago)

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


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.