Issue 764: Switch Pitch Detection Test to use On the Fly Generation instead of file. #783

engineerchuan · 2020-07-14T19:33:41Z

Switch Pitch Detection to on the fly generation.
Refactor the integer encoding that whitenoise and sinusoid uses to be shared.
Refactor test_compliance_kaldi.py. Cannot get rid of kaldi_file_8000.wav because we compare the output to corresponding ark files.

engineerchuan · 2020-07-14T19:39:12Z

Maybe there is no point in trying to use on the fly generation if we have to keep kaldi_file_8000.wav around because we are keeping cached ark files around.

mthrok · 2020-07-14T21:20:30Z

Maybe there is no point in trying to use on the fly generation if we have to keep kaldi_file_8000.wav around because we are keeping cached ark files around.

Yes, the problem is that the code used to generate those ark files are not checked-in so we cannot make modifications to test. If we can recover the code used for those ark files, we can switch the test to completely on-the-fly data generation, which is done for other kaldi compatible tests. #672 #681 #687 #690 and #699.

This situation is very bad because resample is used in other places too resample function has to be tested thoroughly but the data stored as ark provides very limited number of use cases.

test/functional_cpu_test.py

mthrok · 2020-07-14T21:23:42Z

test/functional_cpu_test.py

@@ -300,19 +300,23 @@ def test_linearity_of_istft4(self):

 class TestDetectPitchFrequency(common_utils.TorchaudioTestCase):
    def test_pitch(self):
-        test_filepath_100 = common_utils.get_asset_path("100Hz_44100Hz_16bit_05sec.wav")
-        test_filepath_440 = common_utils.get_asset_path("440Hz_44100Hz_16bit_05sec.wav")
+        SAMPLE_RATE = 44100


Why is this variable capitalized?

I was thinking this is a constant per https://www.python.org/dev/peps/pep-0008/#id48? What case would you prefer?

engineerchuan · 2020-07-15T12:10:48Z

Maybe there is no point in trying to use on the fly generation if we have to keep kaldi_file_8000.wav around because we are keeping cached ark files around.

Yes, the problem is that the code used to generate those ark files are not checked-in so we cannot make modifications to test. If we can recover the code used for those ark files, we can switch the test to completely on-the-fly data generation, which is done for other kaldi compatible tests. #672 #681 #687 #690 and #699.

This situation is very bad because resample is used in other places too resample function has to be tested thoroughly but the data stored as ark provides very limited number of use cases.

Let's address this in a follow up.

2. Relax rtol from 1e-8 to 1e-7 for compliance kaldi 3. Switch to on the fly generation for batch pitch tests

mthrok

Looks good. Added suggestions for further simplification.

test/test_batch_consistency.py

mthrok · 2020-07-15T15:16:07Z

test/common_utils/data_utils.py

+def convert_tensor_encoding(
+    tensor: torch.tensor,
+    dtype: torch.dtype,
+):


…d/common.py

codecov · 2020-07-15T18:21:40Z

Codecov Report

Merging #783 into master will decrease coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #783      +/-   ##
==========================================
- Coverage   89.78%   89.53%   -0.26%     
==========================================
  Files          34       32       -2     
  Lines        2654     2617      -37     
==========================================
- Hits         2383     2343      -40     
- Misses        271      274       +3

Impacted Files	Coverage Δ
torchaudio/_internal/module_utils.py	`85.18% <0.00%> (-11.12%)`	⬇️
torchaudio/sox_effects/sox_effects.py	`94.44% <0.00%> (-0.80%)`	⬇️
torchaudio/__init__.py	`73.33% <0.00%> (ø)`
torchaudio/sox_effects/__init__.py	`100.00% <0.00%> (ø)`
torchaudio/utils/sox_utils.py
torchaudio/utils/__init__.py

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 8181a83...3482273. Read the comment docs.

test/common_utils/test_case_utils.py

test/test_sox_effects.py

…o_backend/common.py" This reverts commit c3967aa.

test/common_utils/test_case_utils.py

mthrok

Looks almost good. One clean up remaining.

mthrok · 2020-07-16T22:10:57Z

Thanks!

engineerchuan added 6 commits July 14, 2020 14:52

convert Sine test to on the fly generation, refactor integer encoding

40fa4f1

avoid files completely for pitch test

4dc5e14

changing compliance kaldi to use on the fly generation

34c2b4f

Refactor compliance kaldi

fe28317

remove typo

9c6d80b

fix

892fa53

mthrok reviewed Jul 14, 2020

View reviewed changes

test/functional_cpu_test.py Outdated Show resolved Hide resolved

mthrok reviewed Jul 14, 2020

View reviewed changes

1. Switch to use parameterized for functional_cpu_test for pitch

4a34565

2. Relax rtol from 1e-8 to 1e-7 for compliance kaldi 3. Switch to on the fly generation for batch pitch tests

engineerchuan changed the title ~~Issue 764: Switch Pitch Detection to On the Fly Generation~~ Issue 764: Switch Pitch Detection Test to use On the Fly Generation instead of file. Jul 15, 2020

relax waveform multi channel resample atol to 1e-7 from 1e-8

a50c392

mthrok reviewed Jul 15, 2020

View reviewed changes

engineerchuan added 3 commits July 15, 2020 12:56

switch to using parameterized across more cases for pitch detection

5a347ed

relax tolerance for length consistency for speed effect

d7e8d49

pull name_func up to test_case_utils and thus deprecate sox_io_backen…

c3967aa

…d/common.py

mthrok reviewed Jul 15, 2020

View reviewed changes

test/common_utils/test_case_utils.py Outdated Show resolved Hide resolved

test/test_sox_effects.py Show resolved Hide resolved

engineerchuan added 3 commits July 15, 2020 20:35

Revert "pull name_func up to test_case_utils and thus deprecate sox_i…

81cbdeb

…o_backend/common.py" This reverts commit c3967aa.

add lambad function for name_func for test pitch in batch

3e90859

removed 2 white noise file, switched one to on the fly generation

b6feb7b

mthrok reviewed Jul 16, 2020

View reviewed changes

test/common_utils/test_case_utils.py Outdated Show resolved Hide resolved

mthrok reviewed Jul 16, 2020

View reviewed changes

engineerchuan and others added 3 commits July 16, 2020 14:34

strip out unused common util for name func

0da56c6

fix

5ea79d6

Merge branch 'master' into issue_764

3482273

mthrok merged commit 02b898f into pytorch:master Jul 16, 2020

mthrok approved these changes Jul 16, 2020

View reviewed changes

engineerchuan deleted the issue_764 branch July 16, 2020 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 764: Switch Pitch Detection Test to use On the Fly Generation instead of file. #783

Issue 764: Switch Pitch Detection Test to use On the Fly Generation instead of file. #783

engineerchuan commented Jul 14, 2020

engineerchuan commented Jul 14, 2020

mthrok commented Jul 14, 2020 •

edited

mthrok Jul 14, 2020

engineerchuan Jul 15, 2020

engineerchuan commented Jul 15, 2020

mthrok left a comment

mthrok Jul 15, 2020

codecov bot commented Jul 15, 2020 •

edited

mthrok left a comment

mthrok commented Jul 16, 2020

Issue 764: Switch Pitch Detection Test to use On the Fly Generation instead of file. #783

Issue 764: Switch Pitch Detection Test to use On the Fly Generation instead of file. #783

Conversation

engineerchuan commented Jul 14, 2020

engineerchuan commented Jul 14, 2020

mthrok commented Jul 14, 2020 • edited

mthrok Jul 14, 2020

Choose a reason for hiding this comment

engineerchuan Jul 15, 2020

Choose a reason for hiding this comment

engineerchuan commented Jul 15, 2020

mthrok left a comment

Choose a reason for hiding this comment

mthrok Jul 15, 2020

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2020 • edited

Codecov Report

mthrok left a comment

Choose a reason for hiding this comment

mthrok commented Jul 16, 2020

mthrok commented Jul 14, 2020 •

edited

codecov bot commented Jul 15, 2020 •

edited