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

Do not use SoxEffectsChain in sox compatibility test #781

Merged
merged 3 commits into from
Jul 14, 2020

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jul 14, 2020

This PR replaces torchaudio.sox_effects.SoxEffectsChain in test_sox_compatibility with bare sox command.

The parity of torchaudio.sox_effects.SoxEffectsChain against sox command is not tested and it has known issues #771, therefore it is not appropriate to use this class for testing other functions.


self.assertEqual(waveform_dithered_noiseshaped, sox_dither_waveform_ns, atol=1e-02, rtol=1e-5)

def test_vctk_transform_pipeline(self):
Copy link
Collaborator 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 because it does not test anything new.

channels and rate effects are no effect because the input file is already one channel with 16k Hz, then the remaining effect is gain -h gain -rh dither -s, but applying gain twice does not make sense (sox command returns error) and dither is tested separately.

@mthrok
Copy link
Collaborator Author

mthrok commented Jul 14, 2020

Resolved

The tests failing on macOS has nothing to do with this PR, and is addressed at #782 .
(This PR touches test/test_sox_compatibility.py, not the failing test/test_sox_effects.py)

@mthrok mthrok marked this pull request as ready for review July 14, 2020 19:08
@mthrok mthrok requested a review from vincentqb July 14, 2020 19:09
@mthrok mthrok force-pushed the replace-test-sox-effects branch from 44f158a to 0eb436b Compare July 14, 2020 20:02
@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #781      +/-   ##
==========================================
- Coverage   89.53%   89.49%   -0.04%     
==========================================
  Files          32       32              
  Lines        2617     2617              
==========================================
- Hits         2343     2342       -1     
- Misses        274      275       +1     
Impacted Files Coverage Δ
torchaudio/_internal/module_utils.py 81.48% <0.00%> (-3.71%) ⬇️

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 131e48b...ddc4153. Read the comment docs.

Comment on lines -89 to -91
"""
Test biquad lowpass filter, compare to SoX implementation
"""
Copy link
Contributor

@vincentqb vincentqb Jul 14, 2020

Choose a reason for hiding this comment

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

nit: Why is the description removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because all the tests in the class have the same description. It's redundant and does not carry new useful information.

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

@mthrok mthrok merged commit db8f2bf into pytorch:master Jul 14, 2020
@mthrok mthrok deleted the replace-test-sox-effects branch July 14, 2020 22:38
@mthrok
Copy link
Collaborator Author

mthrok commented Jul 14, 2020

Thanks

mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023
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