-
-
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
WIP: cleanup blas.jl scal! #8499
Conversation
* fixes issue #8415 * provides a check that `incx == 1` when scaling a complex array with a real number The issue with `scal!` with real scalar and complex array is described with the following example: ```julia using Base.LinAlg.BLAS n = 5 A = ones(n) + im*ones(n) scal!(3,0.0,A,2) ``` Result: ```julia 5-element Array{Complex{Float64},1}: 0.0+1.0im 0.0+1.0im 0.0+1.0im 0.0+1.0im 0.0+1.0im ``` This is also an out of bounds memory access.
Interesting. The build for osx passed, while the build for linux failed. |
The issue with using Base.LinAlg.BLAS
n = 5
A = ones(n) + im*ones(n)
scal!(3,0.0,A,2) Result: 5-element Array{Complex{Float64},1}:
0.0+1.0im
0.0+1.0im
0.0+1.0im
0.0+1.0im
0.0+1.0im This is also an out of bounds memory access. |
Isn't this one ready for merging? The Travis test error is not related to your changes. |
Yes, it can be merged. I thought the solution for the special case of real scalar, complex vector might not be satisfactory. Anyway, it is a step in the right direction. |
Okay. I can see what you mean now. Good catch. Please have a look at this method which does something similar. It might be better to do something similar for the more high level |
I think it would be a good idea to move the real-complex The
Result:
|
Good point. You could consider to define |
Can't even do it at all with current SubArrays, because in general it would be "non-strided": if your step along the first dimension were 4, then when reinterpreting as However, this is doable with the implementation in #8501. |
You could do it with current SubArrays if you increased the dimensionality, though, inserting a new first dimension of size 2. |
The requirement that |
Sorry, I didn't read the whole thing; I was just looking at his |
I see. Then you can get a sorry back from here. I didn't see that @nwh had step sizes of two in his example. That wouldn't be possible to handle for BLAS. |
Indeed. Our generic matrix multiplication could handle it, but you're right it's problematic for BLAS. I suspect this is going to become a recurring theme with our explorations on returning views. There are a lot of operations (like |
For the case of |
Yes, simpler to remove it, since it is not natively supported by BLAS. |
6c7c7e3
to
1a4c02f
Compare
Am I right in saying that it is good to merge this after removing support for the real-scalar complex-array case? |
Yes, I agree that real-scalar complex-array case should be removed. I was going to make a new PR on the updated |
Thanks! Closing this one in that case, and awaiting the new one. |
This pull request addresses the issue discussed in JuliaLang/LinearAlgebra.jl#141.
scal!
is no longer a method forStridedArray
sStridedVector
sscal!
when scaling a complex array with a real number is uncoveredthe technique to save flops assumes
incx == 1
a check for this is added, however this may not be a desirable solution
StridedVector
s