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:

Status badges

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)

trac-9212-identity-zero-mutable.patch (2.2 KB) - added by jason 11 years ago.
trac-9212-fix-doctests.patch (927 bytes) - added by jason 11 years ago.
apply on top of previous patches

Download all attachments as: .zip

Change History (17)

Changed 11 years ago by jason

comment:1 Changed 11 years ago by jason

  • Status changed from new to needs_review

comment:2 Changed 11 years ago by was

  • Resolution set to wontfix
  • Status changed from needs_review to closed

I'm closing this as won't fix, on account of #8276.

comment:3 Changed 11 years ago by mvngu

  • Milestone changed from sage-4.4.4 to sage-duplicate/invalid/wontfix

comment:4 follow-up: Changed 11 years ago by jason

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 jason

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 jason

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 jason

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: Changed 11 years ago by 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? ;-) More commentary in the morning.

Rob

comment:9 Changed 11 years ago by rbeezer

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 jason

  • 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 jason

  • 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 rbeezer

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.

Changed 11 years ago by jason

apply on top of previous patches

comment:13 Changed 11 years ago by jason

Okay, doctest is fixed.

comment:14 Changed 11 years ago by rbeezer

  • 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 mpatel

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