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 )
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-v2.patch, trac_10543-reviewer-rebased.patch
Attachments (4)
Change History (13)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
Forgot to mention - I ran this through all tests, finding only the one random_matrix
example that needed fixing.
Changed 10 years ago by
comment:3 Changed 10 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 10 years ago by
comment:4 Changed 10 years ago by
- 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.
Rob
comment:5 Changed 10 years ago by
- 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/tachyon.py 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
- Milestone set to sage-4.6.2
comment:7 Changed 10 years ago by
- 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).
Changed 10 years ago by
comment:8 Changed 10 years ago by
- 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
- Merged in set to sage-4.6.2.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
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).