-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fixed Ac_mul_Bt and added export and tests, added a missing lapack test #15296
Conversation
travis failure was #15294, restarted
|
let A = [1 3; 5 7], B = [2 4; 3 6] | ||
@test vecdot(A,B) == dot(vec(A),vec(B)) == vecdot_(A,B) == vecdot(float(A),float(B)) | ||
@test vecdot(Int[], Int[]) == 0 == vecdot_(Int[], Int[]) | ||
@test_throws MethodError vecdot(Any[], Any[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it important to test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm being dense, but that line seems to require a BlasReal
and this is Any
. The test seems to call https://codecov.io/github/JuliaLang/julia/base/linalg/generic.jl?ref=1eacbbbb9a686292a12e89bcc4d9559c22928b83#l-270, which is (mostly) covered, and which ends up being a test of whether zero(Any)
is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. I'll remove this for now and we can come back to it later.
CI passed! Anyone have further thoughts? |
@@ -59,6 +59,7 @@ A = rand(1:20, 5, 5) .- 10 | |||
B = rand(1:20, 5, 5) .- 10 | |||
@test At_mul_B(A, B) == A'*B | |||
@test A_mul_Bt(A, B) == A*B' | |||
@test Ac_mul_Bt(A, B) == A'*B.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this actually get tested on complex inputs so we can be sure the conjugation behavior is right?
Hm. We actually don't have parsing for julia> expand(:(A'*B.'))
:(Ac_mul_B(A,transpose(B))) which probably explains why the bug has never surfaced. We should maybe just delete the methods. |
@andreasnoack do you want me to delete the methods, or should we add the parsing? |
Let's delete them. |
No description provided.