Skip to content
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

Migrate kaldi spectrogram #687

Merged
merged 5 commits into from Jun 4, 2020
Merged

Conversation

bhargavkathivarapu
Copy link
Contributor

This PR migrates the kaldi spectrogram test (#597 ) from test/test_compliance_kaldi.py to test/kaldi_compatibility_impl.py

Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #687 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #687   +/-   ##
=======================================
  Coverage   88.79%   88.80%           
=======================================
  Files          22       22           
  Lines        2356     2358    +2     
=======================================
+ Hits         2092     2094    +2     
  Misses        264      264           
Impacted Files Coverage Δ
torchaudio/compliance/kaldi.py 96.09% <100.00%> (+0.02%) ⬆️

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 b4284de...2725ba9. Read the comment docs.

@bhargavkathivarapu
Copy link
Contributor Author

bhargavkathivarapu commented Jun 4, 2020

@mthrok , Applied device and dtype patch to kaldi spectrogram function in kaldi.py . Removed the failing test cases .

Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Mostly looks good.

@@ -284,10 +287,10 @@ def spectrogram(waveform: Tensor,
snip_edges, raw_energy, energy_floor, dither, remove_dc_offset, preemphasis_coefficient)

# size (m, padded_window_size // 2 + 1, 2)
fft = torch.rfft(strided_input, 1, normalized=False, onesided=True)
fft = torch.rfft(strided_input, 1, normalized=False, onesided=True).to(dtype=dtype, device=device)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change does not seem to be necessary.
strided_input and fft should be already on the right dtype/device.
I tried your code without this change and tests ran fine.
Did you find a case this causes RuntimeError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this change,unittest passed.
Before I got runtime error at torch.max step , so safe side I changed device and dtype for inputs to max

@mthrok
Copy link
Collaborator

mthrok commented Jun 4, 2020

I tried running it with CUDA and test_spectrogram_015 and test_spectrogram_107 are failing. Can you remove them too?

$pytest -v test/kaldi_compatibility_cuda_test.py -k spec

_________________ TestKaldi_CUDA_Float32.test_spectrogram_015 __________________

a = (<test.common_utils.TestKaldi_CUDA_Float32 testMethod=test_spectrogram_015>,)

    @wraps(func)
    def standalone_func(*a):
>       return func(*(a + p.args), **p.kwargs)

/home/moto/conda/envs/PY3.8-cuda101/lib/python3.8/site-packages/parameterized/parameterized.py:530:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test/kaldi_compatibility_impl.py:98: in test_spectrogram
    self.assert_equal(result, expected=kaldi_result, rtol=1e-4, atol=1e-8)
test/kaldi_compatibility_impl.py:60: in assert_equal
    self.assertEqual(output, expected, rtol=rtol, atol=atol)
../pytorch/torch/testing/_internal/common_utils.py:1083: in assertEqual
    self.assertTrue(result, msg=message)
E   AssertionError: False is not true : Tensors failed to compare as equal! With rtol=0.0001 and atol=1e-08, found 2 element(s) (out of 18) whose difference(s) exceeded the margin of error (including 0 nan comparisons). The greatest difference was 2.384185791015625e-05 (0.20874881744384766 vs. 0.2087249755859375), which occurred at index (1, 5).
----------------------------- Captured stderr call -----------------------------
compute-spectrogram-feats --blackman-coeff=0.7797 --dither=0 --energy-floor=2.5119 --frame-length=0.625 --frame-shift=0.5 --preemphasis-coefficient=0.69 --raw-energy=true --remove-dc-offset=false --round-to-power-of-two=false --snip-edges=false --subtract-mean=true --window-type=povey scp:- ark:-
LOG (compute-spectrogram-feats[5.5.689~1-2c7e7]:main():compute-spectrogram-feats.cc:157)  Done 1 out of 1 utterances.
_________________ TestKaldi_CUDA_Float64.test_spectrogram_107 __________________

a = (<test.common_utils.TestKaldi_CUDA_Float64 testMethod=test_spectrogram_107>,)

    @wraps(func)
    def standalone_func(*a):
>       return func(*(a + p.args), **p.kwargs)

/home/moto/conda/envs/PY3.8-cuda101/lib/python3.8/site-packages/parameterized/parameterized.py:530:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test/kaldi_compatibility_impl.py:98: in test_spectrogram
    self.assert_equal(result, expected=kaldi_result, rtol=1e-4, atol=1e-8)
test/kaldi_compatibility_impl.py:60: in assert_equal
    self.assertEqual(output, expected, rtol=rtol, atol=atol)
../pytorch/torch/testing/_internal/common_utils.py:1083: in assertEqual
    self.assertTrue(result, msg=message)
E   AssertionError: False is not true : Tensors failed to compare as equal! With rtol=0.0001 and atol=1e-08, found 1 element(s) (out of 28) whose difference(s) exceeded the margin of error (including 0 nan comparisons). The greatest difference was 1.1195481270931396e-06 (0.008482767170622907 vs. 0.00848388671875), which occurred at index (4, 0).
----------------------------- Captured stderr call -----------------------------
compute-spectrogram-feats --blackman-coeff=4.5882 --dither=0 --energy-floor=1.0111 --frame-length=0.375 --frame-shift=0.1875 --preemphasis-coefficient=0.78 --raw-energy=false --remove-dc-offset=true --round-to-power-of-two=false --snip-edges=false --subtract-mean=true --window-type=blackman scp:- ark:-
LOG (compute-spectrogram-feats[5.5.689~1-2c7e7]:main():compute-spectrogram-feats.cc:157)  Done 1 out of 1 utterances.
=========================== short test summary info ============================
FAILED test/kaldi_compatibility_cuda_test.py::TestKaldi_CUDA_Float32::test_spectrogram_015
FAILED test/kaldi_compatibility_cuda_test.py::TestKaldi_CUDA_Float64::test_spectrogram_107
================ 2 failed, 220 passed, 178 deselected in 13.96s ================

Signed-off-by: Bhargav Kathivarapu <bhargavkathivarapu31@gmail.com>
@bhargavkathivarapu
Copy link
Contributor Author

Removed those 2 tests and reverted the device change for fft step

@mthrok
Copy link
Collaborator

mthrok commented Jun 4, 2020

Removed those 2 tests and reverted the device change for fft step

Looks good and verified all tests pass on CUDA. Thanks!

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.

None yet

2 participants