Skip to content

Conversation

krishnakalyan3
Copy link
Contributor

Ref #1414

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

cc @mthrok

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Hi @krishnakalyan3

Thanks for the work. Please refer to my comments.

def test_melscale(self):
sample_rate = 8000
transform = T.MelScale(sample_rate=sample_rate)
waveform = get_whitenoise(sample_rate=sample_rate, duration=0.05, n_channels=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

The input to MelScale has to be spectrogram. Passing waveform here, the MelScale will work on a wrong dimension, so we need to generate spectrogram from waveform and set the proper parameters.

Something like this.

n_fft = 400
n_mels = n_fft // 2 + 1

transform = T.MelScale(sample_rate=sample_rate, n_mels=n_mels)
spec = get_spectrogram(
    get_whitenoise(sample_rate=8000, duration=0.05, n_channels=2),
    n_fft=n_fft, power=1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. To be honest I was not sure about this one.

@mthrok mthrok mentioned this pull request Apr 21, 2021
15 tasks
n_fft = 400
n_mels = n_fft // 2 + 1
transform = T.MelScale(sample_rate=sample_rate, n_mels=n_mels)
waveform = get_spectrogram(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename waveform to spectrogram or spec?
This might appear trivial, but this is rather important for readability.

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@mthrok mthrok merged commit ada50e4 into pytorch:master Apr 23, 2021
@krishnakalyan3 krishnakalyan3 deleted the test_melscale branch April 23, 2021 14:16
@krishnakalyan3
Copy link
Contributor Author

Thanks for the reviews Gentlemen!

carolineechen pushed a commit to carolineechen/audio that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants