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 3 commits
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
28 changes: 28 additions & 0 deletions test/torchaudio_unittest/functional/functional_impl.py
View file
Open in desktop
Expand Up @@ -227,6 +227,34 @@ 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()

@parameterized.expand(
list(itertools.product([(2, 1025, 400), (1, 201, 100)], [100], [0., 30.], [1, 2]))
)
def test_mask_along_axis_preserve(self, shape, mask_param, mask_value, axis):
"""mask_along_axis should not alter original input Tensor

https://github.com/pytorch/audio/issues/1478
"""
torch.random.manual_seed(42)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a tricky one. The goal of this test is to ensure that "when mask is randomly applied, the original Tensor is not altered", and in #1478, we learned that bug happens stochastically. So we need to control the randomness in a way that it always hits the condition that caused the issue in #1478.

Simply setting the random seed has a positive and negative effects here. The positive aspect is that the test is repeatable. Assuming that the environment (hardware / software (including everything from OS, PyTorch and CUDA) is the same, we can repeat the test and expect it to produce the same result. But the negative aspect is, we cannot be sure if this specific seed value applies to any configuration (HW/SW) in future? We do not know and it's not likely. If something about the random generator has been changed in the future, and if this seed value stopped hitting the condition, then the test becomes moot.

There are couple of approaches to overcome this.

  1. Set the test configuration to always hit the correct condition of the reported bug.
    For example, if we make the test to mask all the elements of the input tensor, we can be sure that the test meets the requirement. However, this diverges from the expected usages (should be masking a part of the input, not all), and looking at the signature/docstring of function tested, it's not straightforward to do so. (It could be, but the docstring is hard to understand.)
  2. Patch random generator for the sake of testing.
    If there is no other solution, we can patch the random generator to change the behavior in our favor. This kind of technique is often used in function that relies on external resource (like HTTP access, for example moto makes it possible to test your AWS app without internet connection). But this complicates the test logic, which increases the chance of writing a wrong test.
  3. Bound the probability of this test not hitting the condition.
    Another approach is to bound the probability that the test hits the correct condition. Say that this one attempt will hit the bug condition with probability p. If you repeat the procedure n times, assuming that the implementation of pseudo random generator meets iid, (which I think is a reasonable assumption for this context even though it is not in strict sense) then the probability that the test hits the condition at least once becomes 1 - (1 - p)^n. For p=0.6 and n=10, we get something like 99.989 % of hitting the bug condition. The good thing about this approach is that we can still set the seed value once (and only once at the very beginning) and we expect the reproducibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with everything here, and feel that option 3 makes the most sense to me. The success in #1478 (when the input tensor is not changed) takes place when value = torch.rand(1) * mask_param is equal to 0, which results in mask_start=mask_end and no masking takes place. A mask_param value of 100 in all these tests indicates a 1% probability of value=0 and not hitting the condition. I think therefore looping it over 5 times should be sufficient (> 99.9999%), what do you think? Also, should this reasoning be explained in the documentation or is linking the issue sufficient?

Additionally, I have run this test on the previous implementation and see that all these tests fail, indicating that we do hit the condition (although I do agree that the test as is does not ensure this is always true, if we end up changing parameters or if the state of the random seed changes)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, I have run this test on the previous implementation and see that all these tests fail, indicating that we do hit the condition

Good to know. Thanks for taking the proper care.

A mask_param value of 100 in all these tests indicates a 1% probability of value=0 and not hitting the condition. I think therefore looping it over 5 times should be sufficient (> 99.9999%), what do you think?

Yes, 5 sounds good enough.

Also, should this reasoning be explained in the documentation or is linking the issue sufficient?

Yes, explaining it in the docstring is more helpful for future maintainers. (which is how I learned this kind of technique in the past)

specgram = torch.randn(*shape, dtype=self.dtype, device=self.device)
specgram_copy = specgram.clone()
F.mask_along_axis(specgram, mask_param, mask_value, axis)

self.assertEqual(specgram, specgram_copy)

@parameterized.expand(list(itertools.product([100], [0., 30.], [2, 3])))
def test_mask_along_axis_iid_preserve(self, mask_param, mask_value, axis):
"""mask_along_axis_iid should not alter original input Tensor

https://github.com/pytorch/audio/issues/1478
"""
torch.random.manual_seed(42)
specgrams = torch.randn(4, 2, 1025, 400, dtype=self.dtype, device=self.device)
specgrams_copy = specgrams.clone()
F.mask_along_axis_iid(specgrams, mask_param, mask_value, axis)

self.assertEqual(specgrams, specgrams_copy)


class FunctionalComplex(TestBaseMixin):
complex_dtype = None
Expand Down
6 changes: 5 additions & 1 deletion test/torchaudio_unittest/functional/torchscript_consistency_impl.py
View file
Open in desktop
Expand Up @@ -13,16 +13,20 @@


class Functional(TempDirMixin, TestBaseMixin):
"""Implements test for `functinoal` modul that are performed for different devices"""
"""Implements test for `functional` module that are performed for different devices"""
def _assert_consistency(self, func, tensor, shape_only=False):
tensor = tensor.to(device=self.device, dtype=self.dtype)

path = self.get_temp_path('func.zip')
torch.jit.script(func).save(path)
ts_func = torch.jit.load(path)

torch.random.manual_seed(40)
output = func(tensor)

torch.random.manual_seed(40)
ts_output = ts_func(tensor)

if shape_only:
ts_output = ts_output.shape
output = output.shape
Expand Down
17 changes: 9 additions & 8 deletions torchaudio/functional/functional.py
View file
Open in desktop
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, specgram.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