Skip to content

Merge sox effect and sox_io C++ implementation #779

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 22, 2020
Merged

Merge sox effect and sox_io C++ implementation #779

merged 2 commits into from
Jul 22, 2020

Conversation

@mthrok
Copy link
Contributor

@mthrok mthrok commented Jul 13, 2020 •
edited
Loading

This PR is built on top of #760 , and it merges sox_io backend load/save functions with sox_effects on C++ side. No BC breaking as it's C++ internal.

The number of frames to process at a time (frames_per_chunk parameter of save function) can be adjusted via torchaudio.utils.sox_utils.set_buffer_size, so removed from the function signature. Addressed in #780 separately

mthrok added a commit to mthrok/audio that referenced this pull request Jul 13, 2020
@mthrok
Remove frames_per_chunk argument from save
f051a23 
In pytorch#779, we plan to remove `frames_per_chunk` parameter from `save` function, but it will take some time before we can land pytorch#779, so we go ahead and remove the parameter first to reduce the conflict caused by interface change.
@mthrok mthrok mentioned this pull request Jul 13, 2020
Merged
mthrok added a commit that referenced this pull request Jul 14, 2020
@mthrok
Remove frames_per_chunk argument from save (#780)
4b3e905 
In #779, we plan to remove `frames_per_chunk` parameter from `save` function, but it will take some time before we can land #779, so we go ahead and remove the parameter first to reduce the conflict caused by interface change.
@mthrok mthrok force-pushed the merge-sox-effect-io branch 4 times, most recently from a481617 to f72717d Compare July 14, 2020 20:02
@codecov
Copy link

codecov bot commented Jul 14, 2020 •
edited
Loading

Codecov Report

Merging #779 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #779   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files          35       35           
  Lines        2713     2713           
=======================================
  Hits         2438     2438           
  Misses        275      275

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00cc000...141b569. Read the comment docs.

@mthrok mthrok force-pushed the merge-sox-effect-io branch 2 times, most recently from 5839e44 to b5cd948 Compare July 16, 2020 14:20
@mthrok mthrok marked this pull request as ready for review July 16, 2020 16:27
@mthrok mthrok requested a review from vincentqb July 16, 2020 16:27
@mthrok mthrok force-pushed the merge-sox-effect-io branch 4 times, most recently from 6f2e558 to 2b5463f Compare July 21, 2020 22:06
@vincentqb
Copy link
Contributor

vincentqb commented Jul 22, 2020

merges sox_io backend load/save functions with sox_effects on C++ side

Can you elaborate? I believe this is the change we talked about where frames_per_chunk can be detected?

@mthrok mthrok force-pushed the merge-sox-effect-io branch from 2b5463f to 141b569 Compare July 22, 2020 17:37
@mthrok
Copy link
Contributor Author

mthrok commented Jul 22, 2020

merges sox_io backend load/save functions with sox_effects on C++ side

Can you elaborate? I believe this is the change we talked about where frames_per_chunk can be detected?

This change gets rid of custom implementation for load/save and it uses sox effects chain implementation for them. frames_per_chunk is controlled by sox utils' buffer size.

vincentqb
vincentqb approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, LGTM!

@mthrok mthrok merged commit 0406d30 into pytorch:master Jul 22, 2020
@mthrok mthrok deleted the merge-sox-effect-io branch July 22, 2020 23:04
@mthrok
Copy link
Contributor Author

mthrok commented Jul 22, 2020

thanks

mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023
@yangchris11
Fix grad in-place operation (pytorch#779)
984700d 
p.add_(-lr, p.grad) throws RuntimeError: a leaf Variable that requires grad is being used in an in-place operation, using p.data.add_(-lr, p.grad) fixes this issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@vincentqb vincentqb vincentqb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mthrok @vincentqb