-
Notifications
You must be signed in to change notification settings - Fork 112
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 convolution code to Convolutions.jl submodule #613
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 97.96% 97.96%
=======================================
Files 19 20 +1
Lines 3248 3248
=======================================
Hits 3182 3182
Misses 66 66 ☔ View full report in Codecov by Sentry. |
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. |
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.