Skip to content

Fix time-domain convolution kernel orientation - #3083

Open
Con-Benksl wants to merge 1 commit into
speechbrain:developfrom
Con-Benksl:fix/time-domain-convolution
Open

Con-Benksl wants to merge 1 commit into
speechbrain:developfrom
Con-Benksl:fix/time-domain-convolution

Conversation

@Con-Benksl

Copy link
Copy Markdown

What does this PR do?

Fixes #1527.

convolve1d(..., use_fft=False) currently performs cross-correlation because it passes the kernel directly to PyTorch's conv1d. For a waveform [1, 2, 3, 4] and kernel [1, 2, 4], it returns [17, 24] instead of the valid convolution [11, 18]. Reverse the kernel along its time axis before calling PyTorch.

This intentionally changes time-domain results for asymmetric kernels. DropFreq uses this path both to build and apply its filter; its existing frequency-rejection tests pass, and old/new waveform comparisons agree within floating-point tolerance (bitwise reproducibility is not promised). The FFT path, including the default reverberate implementation, is unchanged.

Validation on CPU with PyTorch 2.6.0:

  • New tests compare against NumPy convolution across padding, stride, dtype, batch and grouped-channel cases, check FFT agreement with linear-convolution padding, and run autograd gradcheck. Before the fix: 24 failed, 1 passed.
  • After the fix: the new tests, existing signal-processing and augmentation tests, and signal-processing doctests all pass: 46 passed.
  • pre-commit run -a and git diff --check pass. No model or dataset downloads were needed. The full repository test suite and GPU execution were not run.

Implemented and tested autonomously with OpenAI Codex, with a separate agent reviewing the code, callers, and compatibility checks.

Before submitting
  • Read the contributor guideline.
  • This PR does one thing.
  • Documentation checked; the existing convolution API description remains applicable.
  • Added necessary regression tests.
  • Verified new and related existing tests locally (scope above).
  • Listed behavior changes for asymmetric time-domain kernels above.
  • Followed project code style and conventions.

PR review

Reviewer checklist
  • Is this pull request ready for review?
  • Check that all items from Before submitting are resolved.
  • Make sure the title is self-explanatory and the description concisely explains the PR.
  • Add labels and milestones (and optionally projects) to classify the PR.
  • Confirm compatibility requirements.
  • Review the self-review checklist.

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.

The rir_waveform should be flipped in reverberate, when use_fft=False?

1 participant