-
Notifications
You must be signed in to change notification settings - Fork 113
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
Move conv to Convolutions.jl or ConvolutionsBase.jl? #615
Comments
Even if we came up with a dispatch where an extension could hook in to do frequency-domain convolution, I wouldn't be happy to see the result of a convolution change depending on whether FFTW is loaded, even if it is within the numerical noise floor.
Sounds easy, but might not be, as it should then also define (by documentation) the exact API. Probably including any dispatch hooks like So maybe it's better to start with a Convolutions.jl package that tries to fulfill the need of any downstream users and provide useful hooks. Once that has stabilized, one can then think about moving the definition of the API to a ConvolutionsBase.jl package. Maybe we should try to bring in more users of convolution to the discussion. E.g. multiplication of polynomials is basically convolution, but Polynomials.jl can't depend on DSP.jl because DSP.jl depends on Polynomials.jl. Would a Convolutions.jl package be helpful for Polynomials.jl, @jverzani? If so, what would you expect of the API? |
I think (among others) ApproxFunBase.jl and Korg.jl also use only |
I can speak for ApproxFunBase. It's fine to ping me 😂 |
|
I think convolution of two |
oh man, I saw jishnub's icon and thought that was it 😅
Hm, currently we don't support convolution of ntuples. @martinholters would it be appropriate to add that? |
Sure, why not? Maybe not in DSP.jl, but in Convolutions.jl, that sounds reasonable. For future reference: for short homogenous tuples, this works quite well: conv(a::Tuple, b::Tuple) = ntuple(n -> sum(a[k]*b[n-k+1] for k in max(1, n-length(b)+1):min(length(a), n)), length(a) + length(b) - 1) We'd probably want to do some |
I think there needs to be an obvious use case for |
It comes up in |
When do polynomials have coefficients of different types? Or are you using it instead of an |
The different types might occur, but primarily we have an option for using an ntuple which speeds up some operations (it was originally borrowed from https://github.com/tkoolen/StaticUnivariatePolynomials.jl). As surmised, an SVector is avoided just because of the dependency. Perhaps we can revisit that decision. |
I haven’t looked at how Polynomials.jl is implemented but supporting tuples seems like it must require a bunch of special cases, eg addition. I suspect it would be simpler to only allow |
Fine with me. Thanks for the consideration. |
Downstream I have a DSP extension just to overload
conv
:https://github.com/JuliaArrays/InfiniteArrays.jl/blob/master/ext/InfiniteArraysDSPExt.jl
I wonder if moving
conv
out would be better. This could take two approaches:conv
with no implementation called ConvolutionsBase.jl. This has the benefit that there is no FFTW.jl dependency.The text was updated successfully, but these errors were encountered: