Opened 11 years ago

Closed 11 years ago

# Echelonize with transformation=True oddness for sparse matrix

Reported by: Owned by: vbraun jason, was major sage-4.7.2 linear algebra sage-4.7.2.alpha1 Volker Braun Rob Beezer N/A

```sage: matrix(ZZ, 3, 4, [1..12], sparse=False).echelon_form(transformation=True)
(
[ 1  2  3  4]  [ 0  2 -1]
[ 0  4  8 12]  [ 0  9 -5]
[ 0  0  0  0], [ 1 -2  1]
)
sage: matrix(ZZ, 3, 4, [1..12], sparse=True).echelon_form(transformation=True)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/vbraun/opt/sage-4.7.1.alpha3/devel/sage-main/<ipython console> in <module>()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/matrix/matrix2.so in sage.matrix.matrix2.Matrix.echelon_form (sage/matrix/matrix2.c:27633)()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/matrix/matrix2.so in sage.matrix.matrix2.Matrix.echelonize (sage/matrix/matrix2.c:27292)()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/matrix/matrix2.so in sage.matrix.matrix2.Matrix._echelonize_ring (sage/matrix/matrix2.c:26560)()

TypeError: Cannot convert tuple to sage.matrix.matrix2.Matrix
```

Appy:

1. trac_11558_echelon_form_with_transformation.patch

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

• Reviewers set to Rob Beezer
• Status changed from new to needs_info

Volker,

Unfortunate - but at least it is fixed now. Passes tests in the obvious places, I'll run long tests overnight.

Comments, in decreasing order of merit.

1. Tests would benefit from a mention of the Trac ticket being fixed.
1. Do you want to document the `transformation` keyword in the docstring?
1. Internal logic of the echelon form code is a mystery to me, as is the code for creating/copying matrices. Since we are both in the neighborhood, is there a faster method for copying a matrix than the following?
```for c from 0 <= c < self.ncols():
for r from 0 <= r < self.nrows():
self.set_unsafe(r, c, d.get_unsafe(r,c))
```

I don't know, just seems inefficient to go entry-by-entry. Not critical, just a thought. Long weekend here in the US, but I'll stick with this.

Rob

### comment:2 Changed 11 years ago by vbraun

1. I think copying elementwise is necessary since the lhs and rhs matrices have potentially different storage order. In particular for sparse matrices, the echelon form is computed with dense matrices and then copied back to a sparse matrix.

It seems like `hermite_form()` is supposed to be an alias for `echelon_form()`. But sparse matrices don't have a `hermite_form()` method? I'll update the patch to fix that and also your points 1 & 2.

Updated patch

### comment:3 Changed 11 years ago by vbraun

• Status changed from needs_info to needs_review

I've added more documentation. Some matrix backends don't support `echelon_form(transformation=True)`, so eventually we should compute the transformation matrix from the pivots if necessary. I'll leave that for later :-)

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

Applies, builds, passes obvious tests on 4.7.1.alpha3. And looks good.

I'll run full tests overnight and then flip this to positive review.

Rob

### comment:5 Changed 11 years ago by rbeezer

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

Passes all long tests on 4.7.1.alpha3, so positive review. Thanks for cleaning this up.

### comment:6 Changed 11 years ago by jdemeyer

• Description modified (diff)

### comment:7 Changed 11 years ago by jdemeyer

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