Opened 11 years ago

Closed 10 years ago

#8094 closed enhancement (fixed)

shortcuts properties for matrix transpose, complex conjugate, conjugate transpose, and inverse

Reported by: schilly Owned by: was
Priority: major Milestone: sage-4.7.2
Component: linear algebra Keywords: sd32
Cc: robertwb, rbeezer Merged in: sage-4.7.2.alpha3
Authors: Harald Schilly, Jason Grout, Martin Raum Reviewers: Rob Beezer, Martin Raum
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

The aim is to enhance the /sage/matrix/matrix2.pyx file with cython's properties to call self.transpose() and other methods more quickly. A shortcut like self.T is resolved to self.transpose(). Shortcuts should be similar to other environments like numpy. Shortcuts added are:

  • m.T -> m.transpose()
  • m.I -> m.inverse()
  • m.C -> m.conjugate()
  • m.H -> m.conjugate().transpose()

Notes: python data model, Cython's properties


Apply:

  1. 8094-shortcut-matrix-transpose-v2-rebased_on_4.7.2.alpha3.patch
  2. trac-8094_matrix_shortcut_docs_reviewer2.patch
  3. trac_8094-matrix-properties-more-documentation-rebased_on_4.7.2.alpha3.patch

Attachments (7)

8094-shortcut-matrix-transpose.patch (6.0 KB) - added by schilly 11 years ago.
doin' it the cython way
8094_matrix_shortcut_docs_reviewer.patch (5.3 KB) - added by jason 10 years ago.
apply on top of previous patches
trac-8094_matrix_shortcut_docs_reviewer2.patch (6.0 KB) - added by mraum 10 years ago.
trac_8094-matrix-properties-more-documentation.patch (3.2 KB) - added by rbeezer 10 years ago.
8094-shortcut-matrix-transpose-v2.patch (5.9 KB) - added by rbeezer 10 years ago.
8094-shortcut-matrix-transpose-v2-rebased_on_4.7.2.alpha3.patch (6.1 KB) - added by leif 10 years ago.
Same patch rebased on (prerelease) Sage 4.7.2.alpha3 because of fuzz 2.
trac_8094-matrix-properties-more-documentation-rebased_on_4.7.2.alpha3.patch (3.4 KB) - added by leif 10 years ago.
Same patch rebased on (prerelease) Sage 4.7.2.alpha3 because of fuzz 2.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 11 years ago by schilly

  • Status changed from new to needs_work

This first patch adds .T for all kinds of matrices in matrix_dense.pyx and some docstrings.

comment:2 Changed 11 years ago by schilly

.T has more tests and I think it works for all of them now.

attachment *.2.patch should be deleted or ignored.

comment:3 Changed 11 years ago by jason

I deleted the .2.patch.

comment:4 Changed 11 years ago by jason

  • Cc robertwb rbeezer added

Since these are cython files, why don't you use the Cython property functionality to implement this?

http://docs.cython.org/docs/extension_types.html#properties

Robert, is there an efficiency gain or some reason why one would prefer the Cython property statement to what is in this patch?

comment:5 Changed 11 years ago by schilly

  • Description modified (diff)

I thought that properties don't work in cython files, but yesterday I tried them (the decorator didn't work, but just the statement is fine) and they do. Thank's for the reference! I'll use them. The advantage is that then you have these shortcuts in autocomplete. It's also better to define them once and for all in matrix2.pyx - I think that's the best place?

comment:6 Changed 11 years ago by schilly

  • Description modified (diff)

Changed 11 years ago by schilly

doin' it the cython way

comment:7 Changed 11 years ago by schilly

  • Description modified (diff)

comment:8 Changed 10 years ago by jason

  • Summary changed from shortcuts for matrix transpose, complex conjugate, ... to shortcuts properties for matrix transpose, complex conjugate, conjugate transpose, and inverse

schilly: is this patch still needs_work, or should it be needs_review now?

comment:9 Changed 10 years ago by schilly

jason: as far as i remember, the problem was that it was interfering with another matrix property (i.e. a doctest was failing and I didn't really understand why). so, the idea as it is coded works, but it's not ready for review.

comment:10 Changed 10 years ago by jason

  • Description modified (diff)

comment:11 Changed 10 years ago by jason

I see one doctest in matrix2.pyx failing, but that's because the expected output was wrong (it's the test for M.H in the conjugate() method).

comment:12 Changed 10 years ago by schilly

ah well, time has passed since i looked into it, but it was a doctest failure somewhere else. probably just another one that has to be modified ... or it isn't an issue any more ;)

comment:13 Changed 10 years ago by jason

I'm uploading a reviewer patch and giving a positive review momentarily :).

comment:14 Changed 10 years ago by schilly

woohoo \o/

Changed 10 years ago by jason

apply on top of previous patches

comment:15 Changed 10 years ago by jason

  • Status changed from needs_work to needs_review

Okay, maybe my changes should be reviewed (since they fix some of the doc issues).

Doctests pass in matrix/*.py[x] with these patches applied, and Harald's code looks good, so positive review for his patch (given that my doctest patch is applied).

Someone should verify my doctest changes are good.

comment:16 Changed 10 years ago by jason

  • Milestone changed from sage-4.6.1 to sage-4.6.2

comment:17 Changed 10 years ago by rbeezer

  • Status changed from needs_review to needs_info

Jason and Harald,

Thanks for putting this together. I'd like to finish up getting this reviewed. But two questions:

  1. It seems like there is evidence of the shortcuts in each of the associated docstrings for the methods, but maybe it isn't always obvious. Like A.I seems to be buried along with the ~ version and has no real explanation that it is a property. I find students get very confused with method calls with empty parentheses, and a property will just add to that, so I think it would be better to have some explanation.
  1. #10471 implements conjugate_transpose() until we reclaim adjoint(). So that would be the place to add in the shortcut. Should we add that here since #10471 has a positive review, and maybe change the definition?

I can do the above on a reviewer patch in the next day or two if necessary.

Rob

comment:18 Changed 10 years ago by mraum

  • Status changed from needs_info to needs_review

I added a further example in the documentation of conjugate_transpose.

I am not sure whether 1. can be addressed at all. Typing m.T? I get the documentation of m. I would suggest to finish this anyway. It is a shame that this patch hasn't been merged for almost 2 years, and one doesn't need to tell students that they type m.T.

comment:19 Changed 10 years ago by rbeezer

  • Authors set to Harald Schilly, Jason Grout, Martin Raum
  • Description modified (diff)
  • Reviewers set to Rob Beezer

These are some of the earliest entries in the reference manual for sage/matrix/matrix2.pyx, so I think the documentation should include some examples, especially since the syntax is different (no parentheses). However, INPUT and OUTPUT blocks feel like make-work, so I have not included that.

I can give a positive review to everything up to my latest patch. If somebody would review my (documentation-only) changes, this could be done.

Rob

comment:20 Changed 10 years ago by mraum

  • Status changed from needs_review to positive_review

Good idea to add this documentation. Everything works fine.

comment:21 Changed 10 years ago by jason

Maybe on another ticket, but can I ask for one more enhancement on this? Can we make the following work:

matrix("""
[1 2]
[3 4]
""")

(i.e., any "]" or "[" next to row delimiters are stripped).  That would mean that I could cut and paste output from Sage back into Sage to make a matrix.

comment:22 follow-up: Changed 10 years ago by jason

Argh. Wrong ticket; please ignore my comment!

comment:23 in reply to: ↑ 22 Changed 10 years ago by rbeezer

Replying to jason:

Argh. Wrong ticket; please ignore my comment!

Whew! ;-)

comment:24 Changed 10 years ago by rbeezer

  • Reviewers changed from Rob Beezer to Rob Beezer, Martin Raum

comment:25 Changed 10 years ago by rbeezer

  • Keywords sd32 added

comment:26 Changed 10 years ago by leif

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

8094-shortcut-matrix-transpose.patch is not a Mercurial changeset.

abort: no username supplied (see "hg help config")

Sorry, Harald. ;)

Changed 10 years ago by rbeezer

comment:27 follow-up: Changed 10 years ago by rbeezer

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

I did some maintenance on the main patch, naming it "v2". Has Harald's user info and is now a proper hg patch. I hope.

Should now be ready to go, since I did not change any code, so should not need a review.

comment:28 in reply to: ↑ 27 Changed 10 years ago by leif

Replying to rbeezer:

I did some maintenance on the main patch, naming it "v2". Has Harald's user info and is now a proper hg patch. I hope.

Yep, sorry. I would have done it myself but failed to recall Harald's e-mail address that moment; then later was simply too busy to search which ticket it was...


Should now be ready to go, since I did not change any code, so should not need a review.

Think so. Only meta data (including line numbers) differ.

As I had other trouble with the release, I think I can still include it.

Changed 10 years ago by leif

Same patch rebased on (prerelease) Sage 4.7.2.alpha3 because of fuzz 2.

Changed 10 years ago by leif

Same patch rebased on (prerelease) Sage 4.7.2.alpha3 because of fuzz 2.

comment:29 Changed 10 years ago by leif

  • Description modified (diff)

Attached rebased versions of two patches because of fuzz 2.

comment:30 Changed 10 years ago by leif

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