Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Ordering by Generalized Eigenvalues for Generalized Schur methods #9701

Merged
merged 3 commits into from
Jan 15, 2015

Conversation

cc7768
Copy link
Contributor

@cc7768 cc7768 commented Jan 9, 2015

This is related to #9655. In #9655, we were hoping to implement the option to pass a function directly to gges/gees so that LAPACK would do everything in the background. After reading a little more and seeing the work that had been done for ordschur in #8467 with the LAPACK function trsen, we decided that a good step would be to implement the corresponding method for the generalized schur method tgsen.

Provides the same functionality as Matlab's ordqz.

CC: @spencerlyon2

@cc7768
Copy link
Contributor Author

cc7768 commented Jan 9, 2015

Is there a reference to selctg in this branch, or are you talking about the other PR (#9655)?

@andreasnoack
Copy link
Member

No you are right. I had both PR open in the browser. I'll delete the message here and post it in the other PR.

@ViralBShah ViralBShah added the linear algebra Linear algebra label Jan 11, 2015
@@ -2,7 +2,7 @@ debug = false

import Base.LinAlg: BlasComplex, BlasFloat, BlasReal, QRPivoted

n = 10
n = 10 # WARNING: n must be >= 10 for GeneralizedSchur tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Is that mentioned somewhere in the LAPACK docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just in all of the tests that relate to the Generalized Schur method (both mine and the original generalized Schur tests), they get two matrices by splitting a into 2 matrices -- a[1:5, 1:5], a[6:10, 6:10]. Just thought it was worth mentioning in case some one ever changed it (the alternative could be to index based on n).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. It would be great if you could change the that to something like 1:div(n,2) and div(n,2)+1:n.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, I think that will be better anyways. I didn't want to make too many changes to the files, I will fix that.

@cc7768
Copy link
Contributor Author

cc7768 commented Jan 12, 2015

I went through lapack.jl and tried to find as many of the stride issues as I could. I think I have fixed at least the majority of them and at least the linalg tests still pass on my computer.

@andreasnoack
Copy link
Member

Great. I can see that it needs a rebase. Could you also squash some of the commits? It would be great with two final commits: one for the ordering and one for that fix of the leading dimensions. If that gets too cumbersome it is okay with a single commit.

@cc7768 cc7768 force-pushed the ordschur_gen branch 2 times, most recently from db44ee5 to a712722 Compare January 12, 2015 19:20
@cc7768
Copy link
Contributor Author

cc7768 commented Jan 12, 2015

I think that I have finally put them all together. I'm not quite a "git-foo" expert, so it took me a few minutes to figure out what I wanted to do (and had a seminar to attend). Before I finished this it looked like it was successfully building on Travis on OSX but not on Linux.

Update: Tests passed with make clean testall on my computer (Ubuntu 14.04)

I created a back up of the branch that I can revert to just in case I shot the whole branch.

@andreasnoack
Copy link
Member

Great. Thanks for the rebase.

Last thing. Could you add some documentation such that people would know how to use the added functionality?

@cc7768
Copy link
Contributor Author

cc7768 commented Jan 14, 2015

No problem. Just added documentation to doc/stdlib/linalg.rst. Let me know if that is sufficient, I tried to be descriptive enough that it is understandable while being brief enough that it isn't too much.

@andreasnoack
Copy link
Member

Great. I'll merge when the lights are green.

andreasnoack added a commit that referenced this pull request Jan 15, 2015
RFC: Ordering by Generalized Eigenvalues for Generalized Schur methods
@andreasnoack andreasnoack merged commit 2c63b5d into JuliaLang:master Jan 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants