Skip to content
This repository has been archived by the owner on Sep 25, 2023. It is now read-only.

fix filtering.resample output for even values of num parameter #517

Merged
merged 46 commits into from
Nov 21, 2022

Conversation

mattkinsey
Copy link
Contributor

@mattkinsey mattkinsey commented Nov 21, 2022

This just fixes the handling of the nyquest frequency bins when resampling using the FFT method (filtering.resample).

Without enforcing the fourier space be hermitian symmetric like this, the resampled output had a few undesirable properties:

  1. It had a different integrated power spectrum than the input
  2. It was not reversible i.e. resample(resample(y, <larger size than y>), <original y size>) != y
  3. The output disagreed with scipy.signal.resample. The max error was around 1e-2 in my application.

Thanks

raydouglass and others added 30 commits March 31, 2020 14:12
[HOTFIX] Fix missing six package on cusignal import [skip-ci-changelog]
[skip ci] Update master references for main branch
@mattkinsey mattkinsey requested a review from a team as a code owner November 21, 2022 01:09
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

1 similar comment
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@awthomp
Copy link
Member

awthomp commented Nov 21, 2022

ok to test

@awthomp awthomp added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 21, 2022
@awthomp awthomp changed the base branch from main to branch-22.12 November 21, 2022 12:17
@awthomp
Copy link
Member

awthomp commented Nov 21, 2022

Hey @mattkinsey ! Thanks for the PR; looks great! I had to change the base branch you were merging into (branch-22.12 rather than main) and fix some minor merge conflicts, but otherwise looks good. We'll get this merged ASAP. Thanks for your contributions, and Happy Thanksgiving!

@awthomp
Copy link
Member

awthomp commented Nov 21, 2022

rerun tests

@awthomp
Copy link
Member

awthomp commented Nov 21, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 5291b62 into rapidsai:branch-22.12 Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants