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

Add dot for complex matrices #22240

Closed
wants to merge 1 commit into from
Closed

Conversation

dalum
Copy link
Contributor

@dalum dalum commented Jun 6, 2017

This fixes the bug mentioned in #22220 where dot is undefined for complex matrices.

@fredrikekre
Copy link
Member

Perhaps add a test? For instance here:

julia/test/blas.jl

Lines 53 to 67 in 5aa15ff

@testset "dot products" begin
if elty <: Real
x1 = convert(Vector{elty}, randn(n))
x2 = convert(Vector{elty}, randn(n))
@test BLAS.dot(x1,x2) sum(x1.*x2)
@test_throws DimensionMismatch BLAS.dot(x1,rand(elty, n + 1))
else
z1 = convert(Vector{elty}, complex.(randn(n),randn(n)))
z2 = convert(Vector{elty}, complex.(randn(n),randn(n)))
@test BLAS.dotc(z1,z2) sum(conj(z1).*z2)
@test BLAS.dotu(z1,z2) sum(z1.*z2)
@test_throws DimensionMismatch BLAS.dotc(z1,rand(elty, n + 1))
@test_throws DimensionMismatch BLAS.dotu(z1,rand(elty, n + 1))
end
end

I did not find any tests for dot(A::Matrix{Real}, B::Matrix{Real}) either, perhaps you can add that in the same block while you're at it?

@andreasnoack
Copy link
Member

andreasnoack commented Jun 6, 2017

Sorry. I should have mentioned in #22220 that dot(Matrix,Matrix) working in the real case is a bug introduced in befbcec. See also #11067 (comment). I think we should deprecate the method in BLAS. You can get that behavior with vecdot.

@fredrikekre
Copy link
Member

That explains the missing tests then.

@stevengj
Copy link
Member

stevengj commented Jun 6, 2017

Alternatively, we could define dot for arbitrary AbstractArray arguments (of matching size), using the ordinary (Euclidean) inner product and add a test.

The main problem with this, however, is that it is inconsistent with the default norm. Of course, we could change the default norm too (which would have the advantage of making the default norm much faster!) but this would be a subtly breaking change.

@dalum
Copy link
Contributor Author

dalum commented Jun 15, 2017

Closing this in light of #22374.

@dalum dalum closed this Jun 15, 2017
@StefanKarpinski
Copy link
Member

Thanks for opening the PR, @eveydee!

@dalum dalum deleted the evey/cmatrixdot branch June 16, 2017 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complex Complex numbers linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants