Move resample to functional and add librosa comparison #1402
Conversation
| lr_upsampled = librosa.resample(waveform.squeeze(0).numpy(), sample_rate, upsample_rate) | ||
| lr_upsampled = torch.from_numpy(lr_upsampled).unsqueeze(0) | ||
|
|
||
| self.assertEqual(ta_upsampled, lr_upsampled, atol=1e-4, rtol=1e-5) |
There was a problem hiding this comment.
@vincentqb @cpuhrsch what's an acceptable margin of error for this comparison? using these values gives an assertion error Tensors failed to compare as equal!With rtol=1e-05 and atol=0.0001, found 28142 element(s) (out of 128000) whose difference(s) exceeded the margin of error (including 0 nan comparisons). The greatest difference was 0.006743520498275757 (0.23187145590782166 vs. 0.2386149764060974), which occurred at index (0, 127992).
There was a problem hiding this comment.
You can look at the kaldi compliance as guidance. As you can see, the tolerances are fairly high there too.
Since we are not changing the algorithm as part of this PR, these new tests are informational, and we take the threshold that makes them pass (after we make sure that we match the correct parameters of course). The threshold therefore becomes new signal/information that we learned from this exercise.
Based on the numbers you gave, assuming we use the correct corresponding parameters for each resampling functions, I'd say atol=1e-2 which is also used here.
There was a problem hiding this comment.
btw @carolineechen, as discussed offline with @cpuhrsch, what other parameters have you tried in librosa's resample? let's follow-up by opening a pull request that calls them explicitly :)
There was a problem hiding this comment.
I had only tried the librosa default values for this test, but yup, I can look into the other parameters
| self.assertEqual(ta_upsampled, lr_upsampled, atol=1e-2, rtol=1e-5) | ||
|
|
||
| ta_downsampled = F.resample(waveform, sample_rate, downsample_rate) | ||
| lr_downsampled = librosa.resample(waveform.squeeze(0).numpy(), sample_rate, downsample_rate) |
There was a problem hiding this comment.
question for future work: why is squeeze needed before passing to librosa?
There was a problem hiding this comment.
have you looked into this @carolineechen ?
There was a problem hiding this comment.
waveform has dimensions (1, n), but librosa.resample requires it to be of shape (n,) or (2, n)
| @@ -649,17 +648,7 @@ def forward(self, waveform: Tensor) -> Tensor: | |||
| Tensor: Output signal of dimension (..., time). | |||
| """ | |||
| if self.resampling_method == 'sinc_interpolation': | |||
There was a problem hiding this comment.
note: we'll leave the discussion around migrating the resampling_method parameter to a follow-up PR
The Building a Convolution/Batch Norm fuser in FX tutorial (https://pytorch.org/tutorials/intermediate/fx_conv_bn_fuser.html) was added for 1.8. This is a minor update to index.rst to have the tutorial show up in the left hand nav under the "Code Transforms with FX" category.
resampletotorchaudio.functional, copied over from kaldi complianceresample_waveformresample_waveform, which now wrapsfunctional.resamplelibrosa.resample