-
Notifications
You must be signed in to change notification settings - Fork 634
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 functionals gain, dither, scale_to_interval #319
Conversation
fe72361
to
5b5473e
Compare
e45a9cb
to
7ea82de
Compare
504f587
to
8f3723d
Compare
c21fec7
to
a703e99
Compare
5c5470c
to
84d40bc
Compare
|
||
def probability_distribution(waveform, density_function="TPDF"): | ||
# type: (Tensor, str) -> Tensor | ||
r"""Apply a probability distribution function on a waveform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be an internal function, and it in fact does the core of applying dither. Let's rename this to _apply_dither
.
nit: "Apply dither to the waveform using the chosen density function."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this comment marked as resolved. What was the resolution of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed the change, I renamed it to _apply_probability_distribution. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this comment... should I still rename to "apply dither"?
2dad290
to
50cf11a
Compare
056d1e2
to
2d601c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost good to go. As mentioned in the comments:
- Let's use a file different from a sine wave for testing, see other assets.
- Let's remove
scale_to_interval
from this PR. - Let's make
probability_distribution
private with a different name like_apply_dither
.
|
||
def probability_distribution(waveform, density_function="TPDF"): | ||
# type: (Tensor, str) -> Tensor | ||
r"""Apply a probability distribution function on a waveform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this comment marked as resolved. What was the resolution of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Good job, I'm merging!
Added functionality for SoX capabilities.
See comment in issue #260 for details.
Gain : Apply amplification or attenuation to the audio signal.
Scale to Interval : Scales waveform to an interval.
Dither: Maximize the dynamic range of audio stored at a particular bit-depth.
Updated VCTK dataset code to replace SoX
is equivalent to