-
Notifications
You must be signed in to change notification settings - Fork 633
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
Remove lazy behavior from MelScale #1636
Conversation
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.
Looks good. Left minor comments on test improvements.
I know these are not directly related to the PR, but otherwise we wouldn't be improving the tests.
@@ -33,13 +33,14 @@ def test_batch_Resample(self): | |||
self.assertEqual(computed, expected) | |||
|
|||
def test_batch_MelScale(self): | |||
specgram = torch.randn(2, 31, 2786) | |||
n_stft = 31 | |||
specgram = torch.randn(2, n_stft, 2786) |
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.
Can you use the default value of n_stft=201
, so that we can catch an error for default value first if that happens in the future?
Also not sure where the 2786 is coming from, but I think we can use something shorter, like 256.
|
||
# Single then transform then batch | ||
expected = torchaudio.transforms.MelScale()(specgram).repeat(3, 1, 1, 1) | ||
expected = torchaudio.transforms.MelScale(n_stft=n_stft)(specgram).repeat(3, 1, 1, 1) |
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.
Note: (follow-up, not the scope of this PR) For batch consistency test, use of repeated does not catch some edge case. #1315
We should update the logic at some point.
@@ -56,7 +56,7 @@ def test_AmplitudeToDB(self): | |||
|
|||
def test_melscale_load_save(self): | |||
specgram = torch.ones(1, 1000, 100) | |||
melscale_transform = transforms.MelScale() | |||
melscale_transform = transforms.MelScale(n_stft=1000) |
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.
Instead of setting n_stft
, can we change the shape of specgram
to 1, 201, 100
?
This is more of a personal preference but 1000
is somehow conspicuous.
Rebases pytorch#1571; addresses pytorch#1569: "In 0.9.0 we are deprecating the lazy behavior of MelScale because it can make an invalid TorchScript object and it does not align with the design of torchaudio. Now in master branch, we can remove the implementation." Co-authored-by: Pankaj Patil <pankaj.patil2099@hotmail.com> Co-authored-by: moto <855818+mthrok@users.noreply.github.com> Co-authored-by: hwangjeff <jeffhwang@fb.com>
Rebases #1571; addresses #1569:
"In 0.9.0 we are deprecating the lazy behavior of MelScale because it can make an invalid TorchScript object and it does not align with the design of torchaudio. Now in master branch, we can remove the implementation."