Add subclass in model test classes #727
Conversation
|
oops, should have waited "ready for review" :) |
test/test_models.py
Outdated
| import torch | ||
| from torchaudio.models import Wav2Letter, _MelResNet | ||
|
|
||
|
|
||
| class TestWav2Letter: | ||
| class TestWav2Letter(unittest.TestCase): |
There was a problem hiding this comment.
To stick with the latest guideline, instead of unittest.TestCase, can you use TorchaudioTestCase from common_utils?
It's available in the latest master.
test/test_models.py
Outdated
| @@ -49,3 +51,6 @@ def test_waveform(self): | |||
| out = model(x) | |||
|
|
|||
| assert out.size() == (batch_size, output_dims, num_features - pad * 2) | |||
|
|
|||
| if __name__ == '__main__': | |||
| unittest.main() | |||
There was a problem hiding this comment.
nit/very subjective: although it does not hurt but I personally find adding unittest.main redundant and unnecessary.
There was a problem hiding this comment.
:) since the other tests have it, let's keep that line in for consistency with the other files and other domains (e.g. here), but feel free to open a pull request to discuss removing them all
There was a problem hiding this comment.
FYI: It was added to the other files without review or approval in #509, and it is not used by any official automated jobs. So it is not very convincing to keep this for the sake of consistency.
There was a problem hiding this comment.
since we don't want to block this PR on this, feel free to open a separate PR to change this everywhere :)
There was a problem hiding this comment.
@jimchen90 -- can you move this change to a separate pull request? thanks!
There was a problem hiding this comment.
Yes. Thanks
Related to #744
Add subclass
common_utils.TorchaudioTestCasein model test classes.Related to #705