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.

Sign up for GitHub

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

Jump to bottom

RFC: add ordschur function to base/linalg #8467

Merged
merged 1 commit into from
Sep 30, 2014
Merged

RFC: add ordschur function to base/linalg #8467

merged 1 commit into from
Sep 30, 2014

Conversation

axsk
Copy link
Contributor

@axsk axsk commented Sep 24, 2014

providing the functionality of matlabs ordschur
http://www.mathworks.de/de/help/matlab/ref/ordschur.html

axsk
axsk reviewed Sep 24, 2014
View reviewed changes
base/linalg/factorization.jl
@@ -659,6 +659,9 @@ function schur(A::AbstractMatrix)
SchurF[:T], SchurF[:Z], SchurF[:values]
end

ordschur!{Ty<:BlasFloat}(Q::StridedMatrix(Ty), T::StridedMatrix(Ty), select::Array(Int)) = Schur(LinAlg.LAPACK.trsen!(select, T , Q)...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line throws an error when i try to compile it in https://gist.github.com/axsk/28f297e2207365a7f4e8,
but i don't know why

edit: ok, also throws an error on travis :( but why?

@axsk axsk mentioned this pull request Sep 24, 2014
Closed
@axsk axsk changed the title WIP: ordschur add ordschur function to base/linalg Sep 25, 2014
@andreasnoack
Copy link
Member

Thank you for contributing this. This looks mainly good, but I'd like the complex version wrapped and some test to check that it works. Also, I think ordschur! should also update the eigenvalue vector.

axsk
axsk reviewed Sep 26, 2014
View reviewed changes
test/linalg1.jl
@@ -286,3 +286,10 @@ end # for eltya
#6941
#@test (ones(10^7,4)*ones(4))[3] == 4.0

debug && println("ordschur")
X = [1 2 3 4; 0 6 7 8; 0 0 11 12; 0 0 0 16]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure where to put this test.
I didn't include a random matrix test, because I see no easy way to automatically select the pairs of eventually occuring conjugate eigenvalues.

@axsk
Copy link
Contributor Author

axsk commented Sep 26, 2014

mh.. even the static test is failing :(
i didnt manage to compile julia myself yet, but when i copy the corresponding definitions to the repl (see https://gist.github.com/axsk/28f297e2207365a7f4e8) on windows 8 the test passes..
so maybe its a platform problem with the representation of LOGICAL in fortran?

@axsk
Copy link
Contributor Author

axsk commented Sep 26, 2014

Running the code from REPL in Julia 0.3 on my local machine (Win64 8.1) it works when using Int and fails with Cint.
On the Travis machines (Linux64 and MacOS64) it works with Cint and fails with Int :|

What shall I do now?
I can't build Julia myself, so could someone with Win64 confirm that this version fails the tests?

@andreasnoack
Copy link
Member

Please try again with BlasInt. Like @jiahao I'm not sure about the size of logical in Fortran and as you have noticed, it probably varies from system to system.

@axsk
Copy link
Contributor Author

axsk commented Sep 26, 2014

it is working! even on my w64 :)
should i remove the static test?
what about the oneliner and the metaprogramming-thing? should I rebase to cleanup history?

@axsk axsk changed the title add ordschur function to base/linalg RFC: add ordschur function to base/linalg Sep 27, 2014
@IainNZ
Copy link
Member

IainNZ commented Sep 27, 2014

Linback to SO question: http://stackoverflow.com/questions/26022244/how-to-convert-a-julia-bool-array-to-fortran-logical-array

@andreasnoack
Copy link
Member

Almost there. Last thing needed before we can merge this is a squash of the commits.

@axsk
Copy link
Contributor Author

axsk commented Sep 29, 2014

into one?

@axsk axsk closed this Sep 29, 2014
@ViralBShah
Copy link
Member

Yes, I think one commit would be Ok.

@axsk axsk reopened this Sep 30, 2014
andreasnoack added a commit that referenced this pull request Sep 30, 2014
@andreasnoack
Merge pull request #8467 from axsk/ordschur
8b88617 
RFC: add ordschur function to base/linalg
@andreasnoack andreasnoack merged commit 8b88617 into JuliaLang:master Sep 30, 2014
@andreasnoack
Copy link
Member

Thank you for the contribution @axsk

@cc7768 cc7768 mentioned this pull request Jan 9, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@axsk @andreasnoack @IainNZ @ViralBShah