Skip to content

Conversation

@harishsdev
Copy link
Contributor

No description provided.

@harishsdev
Copy link
Contributor Author

@nateanl please help to review

@harishsdev harishsdev mentioned this pull request Aug 4, 2021
19 tasks
@harishsdev harishsdev changed the title GriffinLim Audio transform Example GriffinLim Audio transform Aug 4, 2021
@harishsdev harishsdev changed the title Example GriffinLim Audio transform Example code for GriffinLim Audio transform Aug 4, 2021
Example
>>> waveform, sample_rate = torchaudio.load('test.wav', normalize=True)
>>> transform = torchaudio.transforms.GriffinLim(n_fft=800)
>>> transgriffinlim = transform(waveform)
Copy link
Member

@nateanl nateanl Aug 4, 2021

Choose a reason for hiding this comment

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

This looks incorrect. The input of transforms.GriffinLim should be a spectrogram. Could you change the waveform to a spectrogram?

rand_init (bool, optional): Initializes phase randomly if True and to zero otherwise. (Default: ``True``)
Example
>>> waveform, sample_rate = torchaudio.load('test.wav', normalize=True)
Copy link
Member

Choose a reason for hiding this comment

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

The indentation should be 4 spaces after Example. I just realized the space needs to be fixed in MuLawEncoding. But let me fix that.

Example
>>> waveform, sample_rate = torchaudio.load('test.wav', normalize=True)
>>> transform = torchaudio.transforms.GriffinLim(n_fft=800)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> transform = torchaudio.transforms.GriffinLim(n_fft=800)
>>> transform = transforms.GriffinLim(n_fft=800)

@harishsdev harishsdev requested a review from nateanl August 4, 2021 08:17
Comment on lines 159 to 164
Example
>>> waveform, sample_rate = torchaudio.load('test.wav', normalize=True)
>>> transformgri = transforms.GriffinLim(n_fft=800)
>>> transformspec = transforms.Spectrogram(n_fft=800)
>>> x = transformspec(waveform)
>>> transgriffinlim = transformgri(x)
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add transforms.Spectrogram in the example since it returns the complex-type tensor by default. GriffinLim requires the magnitude of the spectrogram which has real type.
Also the Example has indentation issue. Could you make changes in this way:

Suggested change
Example
>>> waveform, sample_rate = torchaudio.load('test.wav', normalize=True)
>>> transformgri = transforms.GriffinLim(n_fft=800)
>>> transformspec = transforms.Spectrogram(n_fft=800)
>>> x = transformspec(waveform)
>>> transgriffinlim = transformgri(x)
Example
>>> batch, freq, time = 2, 257, 100
>>> spectrogram = torch.randn(batch, freq, time)
>>> transform = transforms.GriffinLim(n_fft=512)
>>> waveform = transform(spectrogram)

Comment on lines 159 to 163
Example
>>> batch, freq, time = 2, 257, 100
>>> spectrogram = torch.randn(batch, freq, time)
>>> transform = transforms.GriffinLim(n_fft=512)
>>> waveform = transform(spectrogram)
Copy link
Member

Choose a reason for hiding this comment

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

The indentations still needs to be fixed. There should be only 4 spaces before Example and 8 spaces for the rest.
After fixing the indentation, the PR is good to be merged🙂 @harishsdev.

@harishsdev harishsdev requested a review from nateanl August 4, 2021 10:33
Copy link
Member

@nateanl nateanl left a comment

Choose a reason for hiding this comment

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

Awesome. LGTM!

@nateanl nateanl merged commit 17a3bbf into pytorch:main Aug 4, 2021
nateanl pushed a commit to nateanl/audio that referenced this pull request Aug 4, 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.

3 participants