Opened 10 years ago

Closed 10 years ago

#10543 closed defect (fixed)

Echelon form over QQ is mutable

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-4.6.2
Component: linear algebra Keywords:
Cc: Merged in: sage-4.6.2.alpha4
Authors: Rob Beezer Reviewers: Tom Coates
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by rbeezer)

Which is not the norm for matrices over other rings.

sage: A=matrix(QQ,2,range(4))
sage: B=matrix(ZZ,2,range(4))
sage: C=A.echelon_form()
sage: D=B.echelon_form()
sage: C.is_mutable()
sage: D.is_mutable()



trac_10543-rational-echelon-form-immutable-v2.patch, trac_10543-reviewer-rebased.patch

Attachments (4)

trac_10543-rational-echelon-form-immutable.patch (2.6 KB) - added by rbeezer 10 years ago.
trac_10543-reviewer.patch (2.9 KB) - added by tomc 10 years ago.
trac_10543-reviewer-rebased.patch (2.1 KB) - added by rbeezer 10 years ago.
trac_10543-rational-echelon-form-immutable-v2.patch (2.6 KB) - added by rbeezer 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by rbeezer

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

Patch adds one line of code to set the result to be immutable. A doctest is added to check this behavior, then one other failing doctest in the matrix constructor code is fixed up to allow some subdivisions (which is what brought me here in the first place).

comment:2 Changed 10 years ago by rbeezer

Forgot to mention - I ran this through all tests, finding only the one random_matrix example that needed fixing.

Changed 10 years ago by tomc

comment:3 Changed 10 years ago by tomc

Your random_matrix example fix isn't right: you need to make a copy of the matrix, rather than setting it immutable (it is already immutable!). The reviewer patch fixes this, makes some grammatical changes to the documentation of echelon_form(), and fixes a pre-existing typo in the documentation of echelon_form().

Changed 10 years ago by rbeezer

comment:4 Changed 10 years ago by rbeezer

  • Description modified (diff)
  • Reviewers set to Tom Coates

Hi Tom,

Thanks for the catch on making a copy of the matrix. Not sure what I was thinking just then. Your changes look real good to me.

I think your patch includes my changes and yours mixed together. And some changes get applied in the echelonize routine (which really had me scratching my head). I've tried to split out your changes and have placed a new patch that I belive has just your changes. Except there were some double-colons needed, including one I'd forgotten. You should give the revised patch a real close look.

Can you test the whole package? If so, I think you'd be clear to finalize the review if everything looks OK. It'll need my original patch and then the rebased reviewer patch. I have tested the pair against the two affected files and viewed the HTML documentation, and that much looks good.

Thanks for taking a look at this one and making a fix.


comment:5 Changed 10 years ago by tomc

  • Status changed from needs_review to positive_review

This looks good now. Running:

sage -testall -long

gives that all doctests pass except two unrelated tests (in sage/plot/plot3d/ and sage/plot/plot3d/base.pyx) that also fail in an unpatched copy of Sage (version 4.6.1, built from source on 64-bit Linux).

comment:6 Changed 10 years ago by jdemeyer

  • Milestone set to sage-4.6.2

comment:7 Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This should be rebased to sage-4.6.2.alpha3 + #10028 (or you can wait until sage-4.6.2.alpha4 is released and then rebase to that).

comment:8 Changed 10 years ago by rbeezer

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

Applied #10028 to 4.6.2.alpha3, only got one failure when applying the first of the two patches here. Rebased that patch only, though the failure seems trivial. All tests pass in sage/matrix2.

If this does not work, we'll go for alpha4.

Apply "v2" of original patch, then "reviewer-rebased," in that order.

comment:9 Changed 10 years ago by jdemeyer

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