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

Add autograd tests for TimeMasking/FrequencyMasking #1498

Merged
merged 5 commits into from
May 12, 2021

Conversation

kiri11
Copy link
Contributor

@kiri11 kiri11 commented May 11, 2021

Context: #1414

I combined these two transforms in one test because they're actually the same _AxisMasking transform, but just on different axis. I even thought of testing the _AxisMasking directly, but decided against it because it's not part of the API.

Test would fail without the _DeterministicWrapper likely due to random behaviour here.

Kirill Ignatev added 2 commits May 10, 2021 15:55
1. Added `_DeterministicWrapper`.
2. Combined both masking transforms in one test.
Also test with `iid_masks=True` parameter. Should test with this param as well, since it's a different codepath. 
Make a batch of spectrogram by generating two spectrograms separately and concatenating them.
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.

Hi @kiri11

Thanks for the contribution.

test/torchaudio_unittest/transforms/autograd_test_impl.py Outdated Show resolved Hide resolved
Addressed PR comments
Ensure we're going in the right codepath
@mthrok mthrok merged commit 9d621fd into pytorch:master May 12, 2021
@mthrok
Copy link
Collaborator

mthrok commented May 12, 2021

@kiri11 Thanks!

@mthrok mthrok mentioned this pull request May 12, 2021
15 tasks
@kiri11 kiri11 deleted the time_masking_test branch May 12, 2021 19:59
mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
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.

3 participants