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.

Sign up for GitHub

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

Jump to bottom

N arg convolution #299

Closed
wants to merge 43 commits into from
Closed

N arg convolution #299

wants to merge 43 commits into from

Conversation

HDictus
Copy link
Contributor

@HDictus HDictus commented May 12, 2019 •
edited
Loading

Adresses #224

This PR builds on @galenlynch 's overlap-save branch, so naturally, wait for merging until that is done.
And of course also until I've fixed those tests

galenlynch and others added 30 commits April 17, 2019 08:03
@galenlynch
Overlap-save convolution
f17f6d0 
I have added an overlap-save algorithm to convolution, which is substantially
faster (~2x) for large convolution problems. It is also substantially faster
than other packages like ImageFiltering.jl. For smaller convolutions, it is
about the same or a little bit slower than standard fft convolution. The cutoff
between overlap-save and simple fft convolution is pretty arbitrary at the
moment, so it would be nice to add some benchmarking to allow DSP.jl to choose
between the two algorithms depending on the size of the input. It would also be
nice to add spatial/time domain filtering for small convolutions, as it would
almost certainly be faster than fft convolutions. This is relevant to JuliaDSP#224.
@galenlynch
Fix bug in sizing fft convolution
a0a634f
@galenlynch
Add benchmark script
8974ea4
@galenlynch
Dimensions can have more than two blocks that need to be padded
0b06ca8
@galenlynch
Rename nu and nv in optimalfftfiltlength for consistency
d0531e1
@galenlynch
Use better estimation of fft convolution complexity from JuliaDSP#273
b585889
@galenlynch
Inlining
ea08688
@galenlynch
Remove benchmark script
3f894dd
@galenlynch
Style
5d69cd4
@galenlynch
Some documentation of overlap_save functions
5aa1232
@galenlynch
Test overlap-save
663bc11
@galenlynch
Add comment to data_dest
5c8096c
@galenlynch
Add a little documentation on valid inputs to conv
f958a04
@galenlynch
Properly handle case where OS out is smaller than nffts
cb664e9
@HDictus
Merge branch 'overlap_save_conv' of https://github.com/galenlynch/DSP.jl
6a7c283 
 into overlap_save_conv
@HDictus
Naive N-arg conv in principle working
5c4b61f
@HDictus
not-working naive nargs
947a177
@HDictus
implement N-array fft convolution
535810a 
Not yet passing tests (several issues)
@galenlynch
Overlap-save convolution
710cde3 
I have added an overlap-save algorithm to convolution, which is substantially
faster (~2x) for large convolution problems. It is also substantially faster
than other packages like ImageFiltering.jl. For smaller convolutions, it is
about the same or a little bit slower than standard fft convolution. The cutoff
between overlap-save and simple fft convolution is pretty arbitrary at the
moment, so it would be nice to add some benchmarking to allow DSP.jl to choose
between the two algorithms depending on the size of the input. It would also be
nice to add spatial/time domain filtering for small convolutions, as it would
almost certainly be faster than fft convolutions. This is relevant to JuliaDSP#224.
@galenlynch
Explain _conv_kern_os_edge! parameters
d8a92ae
@galenlynch
Lots of sign posting
8b9a570
@galenlynch
Move edge_dims_arr declaration closer to where it's used
20628f8
@galenlynch
Use fewer loops in perimeter iteration
481c94c
@galenlynch
Speed up testing of conv by using fewer combinations of sizes
9658b7f
@galenlynch
Simplify Val construction
0361993
@galenlynch
Mark _conv_kern_os as unsafe
3d9627c
@HDictus
fix some of the issues
df4b535 
Running into problems with similar
@HDictus
Merge branch 'overlap_save_conv' of https://github.com/galenlynch/DSP.jl
3f13007 
 into N-args
@HDictus
fix most issues, pass most tests
a5f70f5
@HDictus
check against immediate outsize for overlap-save condition
62f4379
@HDictus
Copy link
Contributor Author

HDictus commented May 12, 2019

With regards to type stability, output type depends only on input type so in principle conv(And by extension xcorr) is type-stable, but it seems that the compiler can't infer this. How do I ensure the compiler recognizes this?

@galenlynch
Copy link
Member

It's hard for me to follow what changes you've made to the overlap save branch, because you're merging against master. Is there any way you could rebase your commits on top of my branch so that it's easier to see what's changed?

@galenlynch
Copy link
Member

I'm just sort of browsing through your commits, so I don't know if I have an accurate read on what you've changed. However, if I understand correctly you've changed optimafftfiltlength to operate on an array of input lengths. However, for a convolution like conv(A, Bs...) I think optimalfftfiltlength should still operate on scalar inputs (not an array), and its parameter nb should have the argument maximum(size.(Bs, N)) for each dimension N.

@HDictus
Copy link
Contributor Author

HDictus commented May 14, 2019

It's hard for me to follow what changes you've made to the overlap save branch, because you're merging against master. Is there any way you could rebase your commits on top of my branch so that it's easier to see what's changed?

On this repository, not until it's merged I think, but I can make a PR into overlap-save on your fork, and we can bring the discussion back here later.

@HDictus
Copy link
Contributor Author

HDictus commented May 14, 2019 •
edited
Loading

I'm just sort of browsing through your commits, so I don't know if I have an accurate read on what you've changed. However, if I understand correctly you've changed optimafftfiltlength to operate on an array of input lengths. However, for a convolution like conv(A, Bs...) I think optimalfftfiltlength should still operate on scalar inputs (not an array), and its parameter nb should have the argument maximum(size.(Bs, N)) for each dimension N.

ah ok, nb is supposed to be the maximum, is nx supposed to be the minimum?
How do we know on the overlap-save branch that nb is the maximum? I only see that nb is the dimension of u and nx the dimension of v. u may be larger overall, but not necessarily in every dimension.

@galenlynch
Copy link
Member

ah ok, nb is supposed to be the maximum, is nx supposed to be the minimum?

The names of the parameters comes from this function's use in filt, where x is the large signal and b is the FIR filter taps. nfft will always be more than nb, but it doesn't have to be smaller or larger than x. If you're combining a bunch of small arrays (trailing arrays) to convolve with a larger array (the first array), then you could find nfft for each trailing input (nx is fixed and nb varies) and take the maximum resulting nfft, or equivalently you could find nfft for the maximum nb of all the inputs.

@HDictus
fix optimalfftfiltlength
9547264 
on to the next issue...
@galenlynch galenlynch changed the title N args N arg convolution May 15, 2019
@galenlynch
Copy link
Member

I'll try to take a look at this soon.

@HDictus
Copy link
Contributor Author

HDictus commented May 17, 2019

Thanks Galen. I think I've figured out what was currently causing errors, however for 3 test cases 'naively' performing convolution over all the arguments is significantly faster than this implementation, (one of which is all int, so I think I was wrong earlier about it being due to type differences).
That and the type instability

@HDictus
Copy link
Contributor Author

HDictus commented May 18, 2019 •
edited
Loading

Basing off your branch (and probably those merges I did earlier) has made rebasing a big mess. I'm trying to resolve it by cherry-picking commits onto a new branch - but that's going wrong pretty spectacularly. I think I may just need to manually move all my changes onto a new branch based on overlap-save, which probably means abandoning this PR.

@HDictus
Copy link
Contributor Author

HDictus commented Jan 9, 2020

As I suspected I needed to manually move my changes onto a new branch, closing this and making a new PR

@HDictus HDictus closed this Jan 9, 2020
@galenlynch
Copy link
Member

Sorry...

@HDictus HDictus mentioned this pull request Jan 9, 2020
Open
@HDictus
Copy link
Contributor Author

HDictus commented Jan 9, 2020

Sorry...

Haha, not your fault

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HDictus @galenlynch