[Cherry-picked 0.12] Modify Pitchshift for faster resampling #2441
Conversation
torchaudio/transforms/_transforms.py
Outdated
| self.new_freq = sample_rate | ||
| self.gcd = math.gcd(int(self.orig_freq), int(self.new_freq)) | ||
|
|
||
| if self.orig_freq != self.new_freq: |
There was a problem hiding this comment.
this is outside the scope of this PR, but for someone to follow up on later -- if possible, could be good to integrate the orig_freq == new_freq case into the resampling helper methods to avoid needing this if statement in various code places
|
@skim0514 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
torchaudio/transforms/_transforms.py
Outdated
|
|
||
| shape = waveform.size() | ||
| return _pitch_shift_postprocess( | ||
| stretch, |
There was a problem hiding this comment.
| stretch, | |
| waveform_shift, |
|
As discussed offline, the current implementation ends up being BC-breaking because of the dtype used for the resampling kernel computation (the transform kernel here is computed w/ float64 during initialization, but previously, it is computed w/ the same dtype as the waveform input, which may or may not be float64). We previously ran into this issue for resampling in #1514 and had a workaround in #1556. A better approach to avoid BC-breaking could be using lazy initialization. Let's continue discussion offline to see which solution to take to close the PR. In the meantime, there are still some unaddressed naming/style comments and to move the shape param up as discussed. |
There was a problem hiding this comment.
per offline discussion w/ @mthrok, we can avoid BC-breaking by adopting lazy module for the kernel initialization on top of these changes, which he has implemented and tested in #2468. you can pull in his additional commits on top of yours and merge them together in this PR, thanks for the changes!
I have additionally done some benchmarking and see performance gains with this caching :)
|
Pick up 9db682a on top of the current commit and it should be good to go. |
|
@skim0514 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@skim0514 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Split existing Pitchshift into multiple helper functions in order to cache kernel and speed up overall process addressing pytorch#2359. Existing unit tests pass. edit: functional and transforms unit test pass. Adopted lazy initialization to avoid BC-breaking. Pull Request resolved: pytorch#2441 Reviewed By: carolineechen Differential Revision: D36905582 Pulled By: skim0514 fbshipit-source-id: e07ace8d48764f7839c8877d592650b12bcf667f
|
This pull request was exported from Phabricator. Differential Revision: D36905582 |
|
@skim0514 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Split existing Pitchshift into multiple helper functions in order to cache kernel and speed up overall process addressing #2359. Existing unit tests pass. edit: functional and transforms unit test pass. Adopted lazy initialization to avoid BC-breaking. Pull Request resolved: #2441 Reviewed By: carolineechen Differential Revision: D36905582 Pulled By: skim0514 fbshipit-source-id: 6780db3ac8a29d59017a6abe7e82ce1fd17aaac2
carolineechen
changed the title
Split existing Pitchshift into multiple helper functions in order to cache kernel and speed up overall process addressing #2359.
Existing unit tests pass.
edit: functional and transforms unit test pass. Adopted lazy initialization to avoid BC-breaking.