Opened 10 years ago

Last modified 10 years ago

#11558 closed defect

Echelonize with transformation=True oddness for sparse matrix — at Version 6

Reported by: vbraun Owned by: jason, was
Priority: major Milestone: sage-4.7.2
Component: linear algebra Keywords:
Cc: Merged in:
Authors: Volker Braun Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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

Change History (7)

comment:1 Changed 10 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 10 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.

Changed 10 years ago by vbraun

Updated patch

comment:3 Changed 10 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 10 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 10 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 10 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.