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: |
Description (last modified by )
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:
Attachments (7)
Change History (37)
comment:1 Changed 11 years ago by
- Status changed from new to needs_work
comment:2 Changed 11 years ago by
.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
I deleted the .2.patch.
comment:4 Changed 11 years ago by
- 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
- 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
- Description modified (diff)
comment:7 Changed 11 years ago by
- Description modified (diff)
comment:8 Changed 11 years ago by
- 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 11 years ago by
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 11 years ago by
- Description modified (diff)
comment:11 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
I'm uploading a reviewer patch and giving a positive review momentarily :).
comment:14 Changed 11 years ago by
woohoo \o/
comment:15 Changed 11 years ago by
- 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 11 years ago by
- Milestone changed from sage-4.6.1 to sage-4.6.2
comment:17 Changed 11 years ago by
- 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:
- 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.
- #10471 implements
conjugate_transpose()
until we reclaimadjoint()
. 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
Changed 10 years ago by
comment:18 Changed 10 years ago by
- 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.
Changed 10 years ago by
comment:19 Changed 10 years ago by
- 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
- Status changed from needs_review to positive_review
Good idea to add this documentation. Everything works fine.
comment:21 Changed 10 years ago by
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: ↓ 23 Changed 10 years ago by
Argh. Wrong ticket; please ignore my comment!
comment:23 in reply to: ↑ 22 Changed 10 years ago by
comment:24 Changed 10 years ago by
- Reviewers changed from Rob Beezer to Rob Beezer, Martin Raum
comment:25 Changed 10 years ago by
- Keywords sd32 added
comment:26 Changed 10 years ago by
- 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
comment:27 follow-up: ↓ 28 Changed 10 years ago by
- 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
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.
comment:29 Changed 10 years ago by
- Description modified (diff)
Attached rebased versions of two patches because of fuzz 2.
comment:30 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
This first patch adds .T for all kinds of matrices in matrix_dense.pyx and some docstrings.