-
Notifications
You must be signed in to change notification settings - Fork 115
Move convolution code to Convolutions.jl submodule #613
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #613 +/- ##
=======================================
Coverage 98.13% 98.13%
=======================================
Files 19 20 +1
Lines 3277 3277
=======================================
Hits 3216 3216
Misses 61 61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Not that I'm necessarily opposed to this, but what's the benefit? |
In #598 @dlfivefifty asked if convolutions could be moved out into a separate package. This PR would make it easier to do that, if the decision is made. One benefit would be that those who use only |
I wonder about whether FFTW.jl should be a hard dependency. Alternatively, a simpler package ConvolutionsBase.jl which only defines For example, in InfiniteArrays.jl overloads of https://github.com/JuliaArrays/InfiniteArrays.jl/blob/master/ext/InfiniteArraysDSPExt.jl |
Right, if we want to move the convolutions stuff to a seperate repo, there are a bunch of questions to answer. But I don't think this PR or #598 are the best places to discuss that. Maybe open a dedicated issue or discussion? WRT this PR, I think we should wait until that discussion has settled before actually setting into anything in motion. But as a draft to try things out, it might indeed be helpful. |
5ac3790
to
b8c4fcd
Compare
move nextfastfft to convolutions.jl
as we're already using it in util / others use it too.
Not sure if
xcorr
belongs in there, and currently thedeconv
function callsfilt
, so that can't be moved into convolutions.jl as-is either.