Skip to content

Conversation

@jimchen90
Copy link
Contributor

This is to update the variable names in WaveRNN model.

Related to #735

@jimchen90 jimchen90 requested a review from vincentqb July 17, 2020 17:34
@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #797 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
+ Coverage   89.82%   89.84%   +0.01%     
==========================================
  Files          34       34              
  Lines        2654     2648       -6     
==========================================
- Hits         2384     2379       -5     
+ Misses        270      269       -1     
Impacted Files Coverage Δ
torchaudio/models/_wavernn.py 99.03% <100.00%> (+0.85%) ⬆️

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 102174e...4168fc5. Read the comment docs.

@jimchen90 jimchen90 mentioned this pull request Jul 17, 2020

def test_mol(self):
"""Validate the output dimensions of a _WaveRNN model in mol mode.
"""Validate the output dimensions of a _WaveRNN model in mol loss.
Copy link
Contributor

@vincentqb vincentqb Jul 17, 2020

Choose a reason for hiding this comment

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

nit: "with mol loss"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test has been removed.

Comment on lines 247 to 252
if self.mode == 'waveform':
self.n_classes = 2 ** n_bits
elif self.mode == 'mol':
if self.loss == 'crossentropy':
self.n_classes = n_classes
elif self.loss == 'mol':
self.n_classes = 30
else:
raise ValueError(f"Expected mode: `waveform` or `mol`, but found {self.mode}")
raise ValueError(f"Expected loss: `crossentropy` or `mol`, but found {self.loss}")
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed offline that we would remove the "loss/mode" parameter for WaveRNN. Let's do it as part of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have removed it. I also removed the sample_rate parameter since it is not needed.

Comment on lines 53 to 54
n_hidden: the number of hidden dimensions (default=128)
n_output: the number of output dimensions (default=128)
n_hidden_resblock: the number of hidden dimensions of resblock (default=128)
n_output_melresnet: the number of output dimensions of melresnet (default=128)
Copy link
Contributor

Choose a reason for hiding this comment

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

These are clear as they are for a MelResBlock. Let's keep n_hidden and n_output here, but add "of resblock" and "of melresnet" in the description as you have done here.

Copy link
Contributor Author

@jimchen90 jimchen90 Jul 17, 2020

Choose a reason for hiding this comment

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

I agree, I kept them as before and changed the description.

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

@jimchen90 jimchen90 merged commit 209858e into pytorch:master Jul 17, 2020
@jimchen90 jimchen90 mentioned this pull request Jul 17, 2020
2 tasks
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.

2 participants