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

DSP - A_mul_B!() broken for fft_plan() ? #15781

Closed
Nan-Jia opened this issue Apr 6, 2016 · 8 comments
Closed

DSP - A_mul_B!() broken for fft_plan() ? #15781

Nan-Jia opened this issue Apr 6, 2016 · 8 comments

Comments

@Nan-Jia
Copy link

Nan-Jia commented Apr 6, 2016

Hi,

I'm new to Julia, and currently using the DSP package to do fft. Perhaps I'm not following the syntax to call fft_plan and A_mul_B! correctly?

#A = data[1:512,:,:]  # some data
p = FFTW.plan_fft(A,1)
out = p*A
O = similar(out)
A_mul_B!(O,p,A)
LoadError: MethodError: `A_mul_B!` has no method matching A_mul_B!(::Array{Complex{Float64},3}, ::Base.DFT.FFTW.cFFTWPlan{Complex{Float64},-1,false,3}, ::Array{Float64,3})
Closest candidates are:
  A_mul_B!(::AbstractArray{T,N}, !Matched::Base.DFT.ScaledPlan{T,P,N}, ::AbstractArray{T,N})
  A_mul_B!{T}(::Union{DenseArray{T,N},SubArray{T,N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}}, ::Base.DFT.FFTW.cFFTWPlan{T,K,inplace,N}, !Matched::Union{DenseArray{T,N},SubArray{T,N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}})
  A_mul_B!(::Union{DenseArray{Complex{Float64},N},SubArray{Complex{Float64},N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}}, !Matched::Base.DFT.FFTW.rFFTWPlan{Float64,-1,inplace,N}, ::Union{DenseArray{Float64,N},SubArray{Float64,N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}})
  ...

I should specify I installed the newest version of everything as of 1 week ago, and the output is generated in Jupyter notebook.

@Nan-Jia Nan-Jia changed the title DSP, , A_mul_B!() broken for fft_plan DSP, , A_mul_B!() broken for fft_plan() Apr 6, 2016
@Nan-Jia Nan-Jia changed the title DSP, , A_mul_B!() broken for fft_plan() DSP - A_mul_B!() broken for fft_plan() ? Apr 6, 2016
@stevengj
Copy link
Member

stevengj commented Apr 8, 2016

Do

Ac = complex(A)
A_mul_B!(O, p, Ac)

When you perform an FFT, p*A implicitly first converts A to a complex array before executing the plan. If you use the low-level function A_mul_B! then you must supply complex arrays yourself (there is no implicit conversion).

On the one hand, if you are using A_mul_B! then presumably your intention is to do everything in-place with no temporary arrays, so refusing to silently allocate an array in that case is a good thing. On the other hand, the error message is obviously confusing here, and would be good to improve.

@ivarne
Copy link
Sponsor Member

ivarne commented Apr 12, 2016

Closing this as it is part of a general issue with beginners not understanding MethodError, not a specific issue with A_mul_B!, nor fft_plan.

Improving the error message would require some sort of list of common mistakes. A possible solution is discussed in #7512.

The quick solution would be to define methods like

function A_mul_B!(::Array{Complex{Float64},3}, ::Base.DFT.FFTW.cFFTWPlan{Complex{Float64},-1,false,3}, ::Array{Float64,3})
    error(
        ""
            A_mul_B! is a low level method that does not implicitly convert arguments to Complex.
            do the conversion manually

            eg:
            Ac = complex(A)
            A_mul_B!(O, p, Ac)
        """)
end

but as I understand it, it is not a good idea to define lots of methods that just error with an explanation of why the method should not exist.

@ivarne ivarne closed this as completed Apr 12, 2016
@Nan-Jia
Copy link
Author

Nan-Jia commented Apr 12, 2016

@ivarne @stevengj
Thank you for your replies. I tried the suggested method as follows:

A = data[1:512,:,:]
p = FFTW.plan_fft(A, 1)
Ac = complex(A)
A_mul_B!(Ac,p,A )

And got:

LoadError: MethodError: `A_mul_B!` has no method matching A_mul_B!(::Array{Complex{Float64},3}, ::Base.DFT.FFTW.cFFTWPlan{Complex{Float64},-1,false,3}, ::Array{Float64,3})
Closest candidates are:
  A_mul_B!(::AbstractArray{T,N}, !Matched::Base.DFT.ScaledPlan{T,P,N}, ::AbstractArray{T,N})
  A_mul_B!{T}(::Union{DenseArray{T,N},SubArray{T,N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}}, ::Base.DFT.FFTW.cFFTWPlan{T,K,inplace,N}, !Matched::Union{DenseArray{T,N},SubArray{T,N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}})
  A_mul_B!(::Union{DenseArray{Complex{Float64},N},SubArray{Complex{Float64},N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}}, !Matched::Base.DFT.FFTW.rFFTWPlan{Float64,-1,inplace,N}, ::Union{DenseArray{Float64,N},SubArray{Float64,N,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}})

Is there any reason that the same error should persist here?

@stevengj
Copy link
Member

@NinjaaCat, you are still passing A as the third argument of A_mul_B!. Both the input and output arrays of A_mul_B! should be complex arrays for a complex FFT.

@Nan-Jia
Copy link
Author

Nan-Jia commented Apr 12, 2016

@stevengj Entirely my bad. Thank you!

@Nan-Jia
Copy link
Author

Nan-Jia commented Apr 12, 2016

@stevengj @ivarne Last comment on this thread. I would also suggest clarifying the fft_plan doc, what I did was following this line in the document:

You can also apply a plan with a preallocated output array  by calling A_mul_B!(Â, plan, A)

Thank you both and I'm really liking Julia.

@mbauman
Copy link
Sponsor Member

mbauman commented Apr 12, 2016

but as I understand it, it is not a good idea to define lots of methods that just error with an explanation of why the method should not exist.

With function types, it's now possible to rewrite the large monolithic showerror(::IO, ::MethodError) to dispatch to a method_error_help(::typeof(A_mul_B!), args...). That'd make it nicely extensible for any package.

stevengj added a commit that referenced this issue Apr 12, 2016
@stevengj
Copy link
Member

@NinjaaCat, I've clarified the docs in e5b46c3

@mbauman, that's an interesting idea, maybe open a PR to try it out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants