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

Update InverseMelScale comparison test #1437

Merged
merged 2 commits into from Apr 7, 2021

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Apr 6, 2021

This PR replaces the existing test for InverseMelScale in librosa_compatibliity_test with a new test.

Similar to #1426, test_InverseMelScale in librosa_compatibility_test is not
ensuring the comaptibility to librosa. Having this test can give a wrong statement
about the librosa numerical compatibility of the function.

The new test compares the result of inverse mel scale against the reference spectrogram, so no need to use librosa.
This test serves as more like an insurance that the change to the implementation of InverseMelScale only improves the result, not the other way.

@mthrok mthrok changed the title Remove an invalid InverseMel comparison unit test Update InverseMelScale comparison test Apr 7, 2021
Similar to pytorch#1426 `test_InverseMelScale` in `librosa_compatibility_test` is not
ensuring the comaptibility to librosa. Having this test can give a wrong statement
about the librosa numerical compatibility about the function.
Copy link
Contributor

@carolineechen carolineechen left a comment

Choose a reason for hiding this comment

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

looks good!

@mthrok mthrok merged commit 6db8522 into pytorch:master Apr 7, 2021
@mthrok mthrok deleted the librosa-inversemelscale branch April 7, 2021 20:59
carolineechen pushed a commit to carolineechen/audio that referenced this pull request Apr 30, 2021
* Remove an invalid InverseMel comparison unit test

Similar to pytorch#1426 `test_InverseMelScale` in `librosa_compatibility_test` is not
ensuring the comaptibility to librosa. Having this test can give a wrong statement
about the librosa numerical compatibility about the function.

* Add test for InverseMelScale

The new test compares the result of inverse mel scale against the reference spectrogram, so no need to use librosa.
This test serves as more like an insurance that the change to the implementation of InverseMelScale only improves the result, not the other way.
mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
Amended a typo in part of the description.

Co-authored-by: Brian Johnson <brianjo@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants