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.

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

[BC Breaking] Replace sox_effects init/list/shutdown with TS binding #748

Merged
merged 1 commit into from Jun 25, 2020
Merged

[BC Breaking] Replace sox_effects init/list/shutdown with TS binding #748

merged 1 commit into from Jun 25, 2020

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jun 24, 2020 •
edited

This PR replaces sox effects init/list/shutdown functions with Torch Script binding. (SoxEffectsChain is not changed yet.)
I also moved error checking to C++, so the function signature is changed -> BC breaking.
The previous implementation returned sox_errot_t enum to Python, which is practically useless so I think this BC breaking does not have an impact.

@mthrok mthrok changed the title Replace sox_effects init/list/shutdown with TS binding [BC Breaking] Replace sox_effects init/list/shutdown with TS binding Jun 24, 2020
@mthrok mthrok requested a review from vincentqb June 24, 2020 21:58
@mthrok mthrok marked this pull request as ready for review June 24, 2020 21:58
@vincentqb
Copy link
Contributor

This means that sox_effects would be torchscriptable, right? If so, could you add a test for this?

@mthrok
Copy link
Collaborator Author

mthrok commented Jun 25, 2020

This means that sox_effects would be torchscriptable, right? If so, could you add a test for this?

Nope. the main sox_effects are more complicated so it's not included in this PR, and there is not really way to test init/shutdown/list in the current framework.

vincentqb
vincentqb approved these changes Jun 25, 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.

LGTM

@mthrok mthrok merged commit 0f0d0af into pytorch:master Jun 25, 2020
@mthrok
Copy link
Collaborator Author

mthrok commented Jun 25, 2020

thanks

@mthrok mthrok deleted the sox-effects branch June 25, 2020 22:57
@mthrok mthrok restored the sox-effects branch June 25, 2020 23:08
@mthrok mthrok deleted the sox-effects branch June 25, 2020 23:11
mthrok pushed a commit to mthrok/audio that referenced this pull request Feb 26, 2021
Merge pull request pytorch#748 from jianyuh/jlin27-quant-tutorials
f86f077 
Update Dynamic Quant for BERT tutorial
mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023
@rohan-varma
update (pytorch#748)
496d3b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincentqb vincentqb vincentqb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mthrok @vincentqb