Use parametrize and set random seed in functional batch consistency test

[jcaw]

This PR adds a number of miscellaneous changes to functional/batch_consistency_test.py . They are as follows:

• Parameterize test_sliding_window_cmn so separate tests are generated (also extract the test naming function).

• F.sliding_window_cmn should take a spectrogram, so replace the wave with a spectrogram. (Related to this, F.sliding_window_cmn currently refers to its spectrogram parameter as waveform. Would you like me to change it to spec?)

• Set the manual seed in a couple of places it was missing.

• Tweak the VAD consistency tests by using longer samples for the random test, and using lower thresholds for the file test. This now causes many of the VAD tests to fail.

  This has me thinking - if the purpose of F.vad is to outright crop the input tensor (not, for example, zero it), shouldn't we expect it to produce different results on a sample vs. a batch? I believe my tweaks just contain the right conditions for that to happen, but I might be misreading. If this is the case, should F.vad be removed from the batch consistency tests?

In #1315, @mthrok also mentioned switching the tensor creation to get_whitenoise. I've done this on another branch by introducing a new function get_whitenoise_batch - I wasn't sure if this was the approach you would prefer so I've left it out for now. (For example, I could perhaps add an optional n_items parameter to get_whitenoise which returns a batch iff provided, although that seems messy). I can append those changes to this PR too, and introduce a similar add_spectrogram function if that's also desirable.

[mthrok]

Hi @jcaw

Thanks for the PR. It overall looks good.

• Parameterize test_sliding_window_cmn so separate tests are generated (also extract the test naming function).
• F.sliding_window_cmn should take a spectrogram, so replace the wave with a spectrogram.
• Set the manual seed in a couple of places it was missing.

👍

(Related to this, F.sliding_window_cmn currently refers to its spectrogram parameter as waveform. Would you like me to change it to spec?)

Thanks for catching this. It was an oversight. Let's create a PR for fixing the argument name. It is a BC-breaking change so we might get push back but I think that is the right thing to do.

• Tweak the VAD consistency tests by using longer samples for the random test, and using lower thresholds for the file test. This now causes many of the VAD tests to fail.
  This has me thinking - if the purpose of F.vad is to outright crop the input tensor (not, for example, zero it), shouldn't we expect it to produce different results on a sample vs. a batch? I believe my tweaks just contain the right conditions for that to happen, but I might be misreading. If this is the case, should F.vad be removed from the batch consistency tests?

I see. I did not review the design of F.vad / T.Vad in detail, but your finding sounds like that the design of T.Vad is ill-formed. (I thought that F.vad would zero-out the non-vocal part). To me it does not make sense to batch the results that could be cropped in any way. We should check the behavior of the sox command, but @astaff do you have an idea of how it is supposed to behave?

Anyways, having this change blocks this PR, while the other changes are good improvement. So can you revert the change regards this VAD test failure and file an issue to keep the discussion?

In #1315, @mthrok also mentioned switching the tensor creation to get_whitenoise. I've done this on another branch by introducing a new function get_whitenoise_batch - I wasn't sure if this was the approach you would prefer so I've left it out for now. (For example, I could perhaps add an optional n_items parameter to get_whitenoise which returns a batch iff provided, although that seems messy). I can append those changes to this PR too, and introduce a similar add_spectrogram function if that's also desirable.

While I was skimming through the change, I thought the random tensors were all waveforms, (which was wrong), so I asked if you could change it to get_whitenoise. But as you pointed out it was not waveform and there is no right helper function for getting spectrogram, I think we can keep the original torch.randn pattern, (but maybe fix the shape like you did). For the behavior of get_whitenoise, I think we can default to generate the different noise for each channel. So far I do not see a necessity to have same waveforms across the channel in tests and that can be easily achieved by generating one-channel noise and repeating it across an axis. For spectrogram, I think it's nice and better to have a get_spectrogram helper function.

[jcaw]

No worries, glad to help.

Let's create a PR for fixing the argument name. It is a BC-breaking change so we might get push back but I think that is the right thing to do.

Ah, good point. Ok, I'll create that separately.

can you revert the change regards this VAD test failure and file an issue to keep the discussion?

Certainly. I've removed the two VAD commits and force-pushed (I've just removed work, nothing added). I'll file an issue shortly. (Edit: force pushing seems to have scrubbed the original branch state from the PR, sorry about that. I thought GitHub kept both around. I've copied it here for reference.)

For the behavior of get_whitenoise, I think we can default to generate the different noise for each channel. So far I do not see a necessity to have same waveforms across the channel in tests and that can be easily achieved by generating one-channel noise and repeating it across an axis.

Awesome!

I think we can keep the original torch.randn pattern, (but maybe fix the shape like you did). [...] For spectrogram, I think it's nice and better to have a get_spectrogram helper function.

Ok, that makes sense. Would you like me to introduce the get_spectrogram function in this PR?

[mthrok]

I think we can keep the original torch.randn pattern, (but maybe fix the shape like you did). [...] For spectrogram, I think it's nice and better to have a get_spectrogram helper function.

Ok, that makes sense. Would you like me to introduce the get_spectrogram function in this PR?

Hi @jcaw

I think this PR is good enough improvement so I am going to merge this now. Please open up a new PR.

BTW we released torchaudio 0.8.0 which includes your fix on amplitude_to_DB. Thanks!

[jcaw]

No worries!