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

Change documentation for xcorr and conv to be true to the actual implementation. #521

Closed

Conversation

ErikBuer
Copy link
Contributor

@ErikBuer ErikBuer commented Dec 29, 2023

It looks like there was an intention to implement a clever selection between time domain and frequency domain convolution, but this was never implemented.

The documentation currently says that one of the two domain methods are used, where in reality only FFT convolution is implemented. As it is about 5 years since the functions was last changed, I feel it is time to let go, and just describe what the function does currencty.

If you have any thoughts, I'd love to hear them :)

@wheeheee
Copy link
Member

Doesn't unsafe_conv_kern_os! implement overlap-save?

@ErikBuer
Copy link
Contributor Author

Hmm. I may have mixed this up a bit.

Yes it appears it does "overlap-save?" in ´unsafe_conv_kern_os!´.

I mistok the overlap-save for a time-domain convolution, that was listed as "TODO" in _conv!

# Does convolution, will not switch argument order
function _conv!(out, u, v, su, sv, outsize)
    # TODO: Add spatial / time domain algorithm
    _conv_fft!(out, u, v, su, sv, outsize)
end

Thank you for the correction @wheeheee :)

@ErikBuer ErikBuer closed this Dec 30, 2023
@wheeheee
Copy link
Member

Searched it up, this probably refers to #292

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

Successfully merging this pull request may close these issues.

2 participants