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.

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

Add flanger to functional.py #651

Merged
merged 6 commits into from Jun 2, 2020

Conversation

bhargavkathivarapu
Copy link
Contributor

Hi ,
Regarding the issue ( Reducing dependency in Sox - #260 ),

This pull request is new version of existing pull request #629

@mthrok I have made the changes to batch and torch script test .
Now taking random input generated from a seed

Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
@mthrok
Copy link
Collaborator

mthrok commented May 19, 2020

Thanks, looks like there is something not working for 2D input. The tests are failing. Could you take a look at it?

Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
@bhargavkathivarapu
Copy link
Contributor Author

@mthrok , Fixed the issue , now all checks passed

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.

Overall, LGTM. Minor comments below.


References:
http://sox.sourceforge.net/sox.html
Scott Lehman, Effects Explained, http://harmony-central.com/Effects/effects-explained.html
Copy link
Contributor

@vincentqb vincentqb May 28, 2020

Choose a reason for hiding this comment

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

This link does not work :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original link is not longer working . Found this Archive link on google ( https://web.archive.org/web/20051125072557/http://www.harmony-central.com/Effects/effects-explained.html) , Will submit a separate pull as it referenced in many functions

Comment on lines 1387 to 1392
sinusoidal (bool): If ``True``, uses sinusoidal modulation
If ``False``, uses triangular modulation
(Default: ``True``)
linear_interpolation (bool): If ``True``, uses linear interpolation for delay-line interpolation
If ``False``, uses Quadratic interpolation
(Default: ``True``)
Copy link
Contributor

@vincentqb vincentqb May 28, 2020

Choose a reason for hiding this comment

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

In sox, these flags are passed as strings. I can't find a link with a description of the flags, can you point me to one? Are there more than two possibilities that could happen here? In any case, I may find the flags more explicit. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincentqb Only two possbilities for wave shape and interpolation mentioned in sox source code for flanger (https://github.com/rbouqueau/SoX/blob/master/src/flanger.c) ( check at bottom of file for function arguments description )

Copy link
Contributor

@vincentqb vincentqb May 29, 2020

Choose a reason for hiding this comment

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

Even though there are only two choices, let's use a string with only two values. This will be more explicit. We'll need to raise an error if the parameter is not either.

modulation (str): Use either "sinusoidal" or "triangular" modulation. (Default: ``sinusoidal``)
interpolation (str): Use either "linear" or "quadratic" for delay-line interpolation. (Default: ``linear``)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincentqb , Made the changes , all checks passed

Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
@codecov
Copy link

codecov bot commented May 31, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@ac7c052). Click here to learn what that means.
The diff coverage is 95.08%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #651   +/-   ##
=========================================
  Coverage          ?   89.09%           
=========================================
  Files             ?       21           
  Lines             ?     2284           
  Branches          ?        0           
=========================================
  Hits              ?     2035           
  Misses            ?      249           
  Partials          ?        0           
Impacted Files Coverage Δ
torchaudio/functional.py 95.50% <95.08%> (ø)
torchaudio/datasets/utils.py 50.00% <0.00%> (ø)
torchaudio/datasets/ljspeech.py 85.36% <0.00%> (ø)
torchaudio/models/__init__.py 100.00% <0.00%> (ø)
torchaudio/common_utils.py 60.86% <0.00%> (ø)
torchaudio/transforms.py 95.77% <0.00%> (ø)
torchaudio/compliance/__init__.py 100.00% <0.00%> (ø)
torchaudio/datasets/commonvoice.py 90.00% <0.00%> (ø)
torchaudio/models/wav2letter.py 100.00% <0.00%> (ø)
torchaudio/_sox_backend.py 93.75% <0.00%> (ø)
... and 12 more

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 ac7c052...c8e6848. Read the comment docs.

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 working on this! LGTM!

@vincentqb vincentqb merged commit 9e27cf3 into pytorch:master Jun 2, 2020
mthrok pushed a commit to mthrok/audio that referenced this pull request Feb 26, 2021
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.

None yet

3 participants