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.

Sign up for GitHub

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

Jump to bottom

Ensure axis masking operations are not in-place #1481

Merged
merged 4 commits into from
May 3, 2021
Merged

Ensure axis masking operations are not in-place #1481

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e93578
ensure axis masking is not in-place
Apr 30, 2021
572ae34
modify tests
May 3, 2021
25caed0
bug fix
May 3, 2021
79651b1
bound test error probability
May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions test/torchaudio_unittest/functional/functional_impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ def test_complex_norm(self, shape, power):
def test_mask_along_axis(self, shape, mask_param, mask_value, axis):
torch.random.manual_seed(42)
specgram = torch.randn(*shape, dtype=self.dtype, device=self.device)
specgram_copy = specgram.clone()
mask_specgram = F.mask_along_axis(specgram, mask_param, mask_value, axis)

other_axis = 1 if axis == 2 else 2
Expand All @@ -211,11 +212,13 @@ def test_mask_along_axis(self, shape, mask_param, mask_value, axis):

assert mask_specgram.size() == specgram.size()
assert num_masked_columns < mask_param
self.assertEqual(specgram, specgram_copy)
Copy link
Contributor

Choose a reason for hiding this comment

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

the intention here is to specifically test that the original is not changed. at a minimum, i'd expect a comment justifying why this is here. however, i would usually expect a separate test for this, so we know that this was the intention of the test. having a separate test would also make it easier to extend to other functionals if we want to.

thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with this point. Each unit test should be testing only one thing, so that when a unit test fails we know what aspect of the functionality is not met.

Can you make new test methods with short description and reference the issue number in docstring.
See examples of test descriptions like

audio/test/torchaudio_unittest/backend/sox_io/save_test.py

Lines 381 to 382 in 0c263a9

def test_save_tensor_preserve(self, dtype):
"""save function should not alter Tensor"""

audio/test/torchaudio_unittest/backend/sox_io/load_test.py

Lines 284 to 293 in 0c263a9

def test_mp3(self):
"""Providing format allows to read mp3 without extension
libsox does not check header for mp3
https://github.com/pytorch/audio/issues/1040
The file was generated with the following command
ffmpeg -f lavfi -i "sine=frequency=1000:duration=5" -ar 16000 -f mp3 test_noext
"""


@parameterized.expand(list(itertools.product([100], [0., 30.], [2, 3])))
def test_mask_along_axis_iid(self, mask_param, mask_value, axis):
torch.random.manual_seed(42)
specgrams = torch.randn(4, 2, 1025, 400, dtype=self.dtype, device=self.device)
specgrams_copy = specgrams.clone()

mask_specgrams = F.mask_along_axis_iid(specgrams, mask_param, mask_value, axis)

Expand All @@ -226,6 +229,7 @@ def test_mask_along_axis_iid(self, mask_param, mask_value, axis):

assert mask_specgrams.size() == specgrams.size()
assert (num_masked_columns < mask_param).sum() == num_masked_columns.numel()
self.assertEqual(specgrams, specgrams_copy)


class FunctionalComplex(TestBaseMixin):
Expand Down
17 changes: 9 additions & 8 deletions torchaudio/functional/functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,7 @@ def mask_along_axis_iid(

# Per batch example masking
specgrams = specgrams.transpose(axis, -1)
specgrams.masked_fill_((mask >= mask_start) & (mask < mask_end), mask_value)
specgrams = specgrams.masked_fill((mask >= mask_start) & (mask < mask_end), mask_value)
specgrams = specgrams.transpose(axis, -1)

return specgrams
Expand All @@ -772,24 +772,25 @@ def mask_along_axis(
Returns:
Tensor: Masked spectrogram of dimensions (channel, freq, time)
"""
if axis != 1 and axis != 2:
raise ValueError('Only Frequency and Time masking are supported')

# pack batch
shape = specgram.size()
specgram = specgram.reshape([-1] + list(shape[-2:]))

value = torch.rand(1) * mask_param
min_value = torch.rand(1) * (specgram.size(axis) - value)

mask_start = (min_value.long()).squeeze()
mask_end = (min_value.long() + value.long()).squeeze()
mask = torch.arange(0, shape[axis], device=specgram.device, dtype=specgram.dtype)
mask = (mask >= mask_start) & (mask < mask_end)
if axis == 1:
mask = mask.unsqueeze(-1)

assert mask_end - mask_start < mask_param
if axis == 1:
specgram[:, mask_start:mask_end] = mask_value
elif axis == 2:
specgram[:, :, mask_start:mask_end] = mask_value
else:
raise ValueError('Only Frequency and Time masking are supported')

specgram = specgram.masked_fill(mask, mask_value)

# unpack batch
specgram = specgram.reshape(shape[:-2] + specgram.shape[-2:])
Expand Down