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

Fix type instability in logdet(::Matrix{Complex64}) #10189

Merged
merged 1 commit into from
Feb 14, 2015
Merged

Fix type instability in logdet(::Matrix{Complex64}) #10189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions base/linalg/lu.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,12 @@ end

function logdet{T<:Complex,S}(A::LU{T,S})
n = chksquare(A)
s = sum(log(diag(A.factors))) + (bool(sum(A.ipiv .!= 1:n) % 2) ? complex(0,pi) : 0)
s = sum(log(diag(A.factors)))
if bool(sum(A.ipiv .!= 1:n) % 2)
s = Complex(real(s), imag(s)+π)
end
r, a = reim(s)
a = pi-mod(pi-a,2pi) #Take principal branch with argument (-pi,pi]
a = π-mod2pi(π-a) #Take principal branch with argument (-pi,pi]
complex(r,a)
end

Expand Down
1 change: 1 addition & 0 deletions test/linalg/lu.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@test logdet(Complex64[1.0f0 0.5f0; 0.5f0 -1.0f0]) === 0.22314355f0 + 3.1415927f0im
Copy link
Contributor

Choose a reason for hiding this comment

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

does this really need to be its own 1-line file?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is in line with @andreasnoack's work in reorganizing the linalg tests by source file. lu.jl simply hadn't been done yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to move chunks of other pre-existing tests into this file, sure that makes sense. A 1-line file is pretty pointless and not worth the noise.

Copy link
Member

Choose a reason for hiding this comment

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

Would be worth it if there were more tests, which there should be. So maybe leave it in anticipation of more LU tests?

Copy link
Member

Choose a reason for hiding this comment

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

The other LU tests are at different places in linalg1-4.jl and at some point they should go into the new lu.jl, but I don't think we should hold back this fix by requiring the reorganization to take place here. We should probably also add some more LU test.

2 changes: 1 addition & 1 deletion test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ end
if "linalg" in tests
# specifically selected case
filter!(x -> x != "linalg", tests)
prepend!(tests, ["linalg1", "linalg2", "linalg3", "linalg4", "linalg/lapack", "linalg/triangular", "linalg/tridiag", "linalg/pinv", "linalg/givens"])
prepend!(tests, ["linalg1", "linalg2", "linalg3", "linalg4", "linalg/lapack", "linalg/triangular", "linalg/tridiag", "linalg/pinv", "linalg/givens", "linalg/lu"])
end

net_required_for = ["socket", "parallel"]
Expand Down