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

Clarify use of cross-correlation for FIR filters #1768

Merged
merged 1 commit into from
May 1, 2024

Conversation

glopesdev
Copy link
Member

@glopesdev glopesdev commented Apr 30, 2024

The underlying implementation of the FirFilter operator uses the filter2D function from OpenCV to efficiently apply the filter.

Although the documentation of filter2D itself states that it "convolves an image with a kernel", in fact the implementation actually computes the cross-correlation. This is a surprisingly common, if misleading, assumption in the computer vision and image processing field, likely coming from the use of either symmetric kernels (in which case correlation is equal to convolution), or learned kernels (in which case the learning process can easily deal with reversed kernel weights).

Nevertheless, the current reference documentation of the operator can be misleading if one is attempting to use FirFilter with externally computed asymmetric kernels for temporal convolution. This PR adds a new remarks section clarifying the relationship between correlation and convolution, and how the kernel must be processed to obtain the correct result in case of an asymmetric kernel.

Fixes #1761

@glopesdev glopesdev added documentation Improvements or additions to documentation fix Pull request that fixes an issue labels Apr 30, 2024
@glopesdev glopesdev added this to the 2.8.3 milestone Apr 30, 2024
@glopesdev glopesdev requested a review from J-M-White April 30, 2024 23:11
@glopesdev glopesdev merged commit ced27db into bonsai-rx:main May 1, 2024
3 checks passed
@glopesdev glopesdev deleted the issue-1761 branch May 1, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FirFilter (DSP) computes correlation, not convolution
2 participants