Add simulate_rir_ism method for simulating RIR with Image Source Method #2644
Conversation
|
cc @fakufaku |
test/torchaudio_unittest/prototype/functional/functional_test_impl.py
Outdated
Show resolved
Hide resolved
test/torchaudio_unittest/prototype/functional/functional_test_impl.py
Outdated
Show resolved
Hide resolved
| @skipIfNoModule("pyroomacoustics") | ||
| @parameterized.expand([(2, 1), (3, 4)]) | ||
| def test_simulate_rir_ism_single_band(self, D, channel): | ||
| """Test simulate_rir_ism when absorption coefficients are identical for all walls.""" |
There was a problem hiding this comment.
I do not see the test logic checks the identity described in the docstring.
It is checking against pyroomacoustic, but the test logic does not check identity for walls.
Can you elaborate?
There was a problem hiding this comment.
Yes. if e_absorption is just a floating-point value, then it means it is identical to all walls. That's why the room in pyroonacoustics is constructed with materials=pra.Material(e_absorption),.
The difference of this test and the one below is if e_absorption is a 2D Tensor, the multi-band logic in https://github.com/pytorch/audio/pull/2644/files#diff-bfbe08000c5dc05901af3bad9503693a99068426351748925a322990daed0cb3R217-R222 is applied. We need a separate test for that.
| const int64_t ir_length = irs.size(3); | ||
| torch::Tensor rirs = | ||
| torch::zeros({num_band, num_mic, rir_length}, irs.dtype()); | ||
| rirs.requires_grad_(true); |
There was a problem hiding this comment.
Does this work with torch.inference_mode()?
There was a problem hiding this comment.
actually, what does it accomplish?
build_rir_impl manipulates the data of rirs by pointer, so autograd cannot track it.
In this case, the modification of requires_grad=True should be done by client code.
There was a problem hiding this comment.
I tried to remove this line and run autograd_test, the test fails but was working with this line.
There was a problem hiding this comment.
What happens if you move require_grad to after the call to build_rir_impl ?
There was a problem hiding this comment.
It also passes the autograd test.
There was a problem hiding this comment.
That suggests that the call to rirs.requires_grad_(true) can be outside of this function. Perhaps can you set it in test code?
| torch::Tensor& filters) { | ||
| int64_t n = centers.size(0); | ||
| torch::Tensor new_bands = torch::zeros({n, 2}, centers.dtype()); | ||
| new_bands.requires_grad_(true); |
There was a problem hiding this comment.
Similar to above, I double the effectiveness of requires_grad_ here.
| return img_loc, att | ||
|
|
||
|
|
||
| def _hann(x: torch.Tensor, T: int): |
There was a problem hiding this comment.
Can you add comment on why we cannot use torch.hann_window?
There was a problem hiding this comment.
sue. The reason is the original formula of image source method truncate the function values, any points outside [-T//2. T//2] is 0.
There was a problem hiding this comment.
Actually, torch.hann_window can only sample the window function at integer points, which is suitable to create the window functions for STFT and such. In the image source model, we need to sample the continuous window function at non-integer points.
There was a problem hiding this comment.
@nateanl This is not addressed. We need an explanation of why this function is required.
| return torch.special.sinc(n - delay) * _hann(n - delay, 2 * pad) | ||
|
|
||
|
|
||
| def simulate_rir_ism( |
There was a problem hiding this comment.
Can you decorate this function so that if RIR is not compiled, it will fail immediately with easy-to-understand message?
| "torchaudio::build_rir(Tensor irs, Tensor delay_i, int rir_length) -> Tensor", | ||
| &torchaudio::rir::build_rir); | ||
| m.def( | ||
| "torchaudio::make_filter(Tensor centers, float sample_rate, int n_fft) -> Tensor", |
There was a problem hiding this comment.
The function name is very generic to be in torchaudio namespace. Can you make it more descriptive / specific?
| using namespace torch::indexing; | ||
|
|
||
| namespace torchaudio { | ||
| namespace rir { |
There was a problem hiding this comment.
Since this file does not have corresponding header file, can you add anonymous namespace so that it is not referable from other C++ source?
| @@ -1,9 +1,14 @@ | |||
| import numpy as np | |||
|
|
|||
| try: | |||
| import pyroomacoustics as pra | |||
There was a problem hiding this comment.
Please follow
audio/test/torchaudio_unittest/backend/soundfile/info_test.py
Lines 20 to 21 in fda00bf
|
After discussing with @fakufaku and @NicolasHug, the gradients of converting a float tensor to int tensor and using it as indices for another tensor may require an analytic formula. Before getting the formula with proof, I will disable the autograd test and focus on the numerical correctness in the forward method. |
|
will address the issue in a new PR #2880 |
No description provided.