Opened 12 years ago
Last modified 12 years ago
#10543 closed defect
Echelon form over QQ is mutable — at Version 4
Reported by: | Rob Beezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.6.2 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | ||
Authors: | Rob Beezer | Reviewers: | Tom Coates |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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() True sage: D.is_mutable() False
See http://groups.google.com/group/sage-devel/browse_thread/thread/ef90d24da0ee704c
Apply
trac_10543-rational-echelon-form-immutable.patch, trac_10543-reviewer-rebased.patch
Change History (7)
Changed 12 years ago by
Attachment: | trac_10543-rational-echelon-form-immutable.patch added |
---|
comment:1 Changed 12 years ago by
Authors: | → Rob Beezer |
---|---|
Status: | new → needs_review |
comment:2 Changed 12 years ago by
Forgot to mention - I ran this through all tests, finding only the one random_matrix
example that needed fixing.
Changed 12 years ago by
Attachment: | trac_10543-reviewer.patch added |
---|
comment:3 Changed 12 years ago by
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 12 years ago by
Attachment: | trac_10543-reviewer-rebased.patch added |
---|
comment:4 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | → 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.
Rob
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).