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

Run transform librosa compatibility test on CUDA as well #1439

Merged
merged 1 commit into from Apr 8, 2021

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Apr 7, 2021

Similar to #1436 this PR ports librosa compatibility test for Transforms to CPU/CUDA.

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.

left a minor comment/question, but otherwise lgtm

from .librosa_compatibility_test_impl import TransformsTestBase


class TestTransforms(TransformsTestBase, PytorchTestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to rename this TestTransformsCPU (and similarly for the CUDA test)? I see it being done both ways in existing unit tests (with, without) but maybe this is something we want to keep consistent moving forward

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I started out as very verbose test class name but in #1436, it was pointed out that I forgot to change it from CPU to CUDA when I copy pasted it, and that made me wonder if we need to be that verbose for this. CPU tests and CUDA tests are separated on file-basis, so there is not a case where you look at test log and do not know which device it is running the test.
So I am learning towards removing CPU/CUDA from the name of test class.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that makes sense to me. We can remove CPU/CUDA from test class names moving forward

@mthrok mthrok merged commit de78619 into pytorch:master Apr 8, 2021
@mthrok mthrok deleted the test-librosa branch April 8, 2021 00:46
carolineechen pushed a commit to carolineechen/audio that referenced this pull request Apr 30, 2021
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