Opened 11 years ago
Closed 11 years ago
#9212 closed defect (fixed)
top-level identity_matrix and zero_matrix functions should return mutable matrices
Reported by: | jason | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.5.2 |
Component: | linear algebra | Keywords: | |
Cc: | rbeezer, florent | Merged in: | sage-4.5.2.alpha0 |
Authors: | Jason Grout | Reviewers: | Rob Beezer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
After #8276, the top-level identity_matrix and zero_matrix functions return immutable matrices, which is a backwards-incompatible change that is inconvenient in many cases (when code starts with those matrices as the base and then modifies them).
This patch makes these matrices mutable.
Attachments (2)
Change History (17)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Resolution set to wontfix
- Status changed from needs_review to closed
comment:3 Changed 11 years ago by
- Milestone changed from sage-4.4.4 to sage-duplicate/invalid/wontfix
comment:4 follow-up: ↓ 5 Changed 11 years ago by
The discussion there didn't discuss the top-level identity_matrix or zero_matrix functions. In fact, they made a big change to those functions without deprecations. The attached patch does *not* revert the changes in #8276. It just restricts the changes to exactly what was discussed (i.e., this patch takes care of what I see as an unintended ramification of the changes in #8276).
I won't reopen this, as that's pushing things. But I think you're cutting off the discussion way too soon.
comment:5 in reply to: ↑ 4 Changed 11 years ago by
Replying to jason:
I won't reopen this, as that's pushing things. But I think you're cutting off the discussion way too soon.
(I mean, I won't personally reopen this right now, but I really wish you would revert your decision to shut it down so abruptly, especially considering that this ticket does *not* revert the changes in #8276, and in fact makes #8276 not break the deprecation policy).
comment:6 Changed 11 years ago by
Yet another reason for making identity_matrix and zero_matrix behave as they always have behaved and return mutable matrices: every top-level constructor from matrix/constructor.py (e.g., matrix(...), random_matrix(...), block_matrix(...), etc.) used to return a mutable matrix, so they were consistent with each other. But now, #8276 made the top-level identity_matrix and zero_matrix constructors behave unlike the others (again, without deprecation and without discussion of the top-level function behavior).
comment:7 Changed 11 years ago by
I posted a call for a vote to sage-devel:
http://groups.google.com/group/sage-devel/browse_thread/thread/b3743044a4579376
because clearly if the confusion above stemmed from a discussion on sage-devel, the appropriate place to resolve things is on sage-devel.
comment:8 follow-up: ↓ 11 Changed 11 years ago by
Hey Jason,
Following test in sage/misc/sagedoc.py (line 1089) is now broken.
sage: browse_sage_doc(identity_matrix, 'rst')[-60:-5]
You did run full tests before posting this, didn't you? ;-) More commentary in the morning.
Rob
comment:9 Changed 11 years ago by
I have a review of this ready to go, and the only change needed is to fix the doctest doctest failure noted above. Once you are satisfied with the vote on sage-devel, note the result here and I'll proceed accordingly.
Rob
comment:10 Changed 11 years ago by
- Milestone changed from sage-duplicate/invalid/wontfix to sage-4.4.4
- Resolution wontfix deleted
- Status changed from closed to new
I'm satisfied with the vote enough to reopen this ticket.
comment:11 in reply to: ↑ 8 Changed 11 years ago by
- Status changed from new to needs_review
Replying to rbeezer:
Hey Jason,
Following test in sage/misc/sagedoc.py (line 1089) is now broken.
sage: browse_sage_doc(identity_matrix, 'rst')[-60:-5]You did run full tests before posting this, didn't you? ;-)
No, I didn't run full doctests before posting the patch, so thanks for catching this! I believe I tested the matrix directory, though.
When you say you have a review ready to go, do you mean that you have a reviewer patch to fix the doctest error, or should I do that?
comment:12 Changed 11 years ago by
I have a "positive review" report, but no reviewer patch to make it happen. So make/update a patch, and I'll issue a review.
comment:13 Changed 11 years ago by
Okay, doctest is fixed.
comment:14 Changed 11 years ago by
- Reviewers set to Rob Beezer
- Status changed from needs_review to positive_review
This all looks good, passes tests on 4.4.4.alpha0 and HTML documentation looks fine coming from matrix/constructor.py
Positive review.
comment:15 Changed 11 years ago by
- Merged in set to sage-4.5.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
I'm closing this as won't fix, on account of #8276.