-
Notifications
You must be signed in to change notification settings - Fork 739
feat: batching/filter banks support of lfilter #1561
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.
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
Conversation
update to new version
Accommodate changes in lfilter
Rebase to master
Get synced
|
Hi @yoyololicon Thanks for the PR. This is another wonderful addition. Can you add a batch consistency test?
|
Sure.
I'm not sure whether the
>>> x = torch.randn(44100)
>>> a_coeffs = torch.rand(2, 3)
>>> b_coeffs = torch.rand(2, 3)
>>> F.lfilter(x, a_coeffs, b_coeffs).shape
(2, 44100)The signal has been filtered by a set of filters, corresponding to the feature proposed in #1528 .
>>> x = torch.randn(2, 44100)
>>> a_coeffs = torch.rand(2, 3)
>>> b_coeffs = torch.rand(2, 3)
>>> F.lfilter(x, a_coeffs, b_coeffs).shape
(2, 44100)In this example, each signal is filtered by its own set of filter coefficients, corresponding to the batching feature proposed in #1476.
>>> x = torch.randn(10, 1, 44100)
>>> a_coeffs = torch.rand(2, 3)
>>> b_coeffs = torch.rand(2, 3)
>>> F.lfilter(x, a_coeffs, b_coeffs).shape
(10, 2, 44100)Batches of signals are filtered by a set of filters.
>>> x = torch.randn(10, 2, 44100)
>>> a_coeffs = torch.rand(2, 3)
>>> b_coeffs = torch.rand(2, 3)
>>> F.lfilter(x, a_coeffs, b_coeffs).shape
(10, 2, 44100)In this example, each channel of the signals is filtered by different coefficients. I might benchmark the filter speed later this weekend cuz my machine is running some training these days. |
|
@yoyololicon By In this case, to get the resulting filtered signal2, I need to do the slicing and to do that I need to know the length of the resulting signal. i.e. However, looking at the examples you showed, |
@mthrok
According to above explaination, you are right. |
|
@yoyololicon Thanks for the confirmation. Looking at the tests, I think the PR looks okay behavior-wise. Let me look into the code detail soon. |
| for i in range(self.batch_size) | ||
| ]) | ||
|
|
||
| self.assertEqual(batchwise_output, itemwise_output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use self.assert_batch_consistency helper method? It handles dtype/device as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assert_batch_consistency seems assume it only needs to take batch on the first input, but in our case, a_coeffs and b_coeffs should also be in batch as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean it is applying different filters to samples in batch? I thought the same set of filters are applied to each sample in batch, so one can change the batch size without changing a_coeffs and b_coeffs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we need to do 2 type of tests. I will add another one that use self.assert_batch_consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And can you clarify that in the above case, where you said "a_coeffs and b_coeffs should also be in batch as well", the number of filter bank happens to be same as the batch size, but that's not requirement?
So my understanding/expectation is that when input batch is the shape of [batch_size, sequence_length], a_coeffs and b_coeffs can take any shape of [filter_dim, number_of_filters], without being constrained on the input shape.
And if I understand correctly, here your test is testing that filter banks produces the same result regardless they are applied separately or together, in that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my understanding/expectation is that when input batch is the shape of
[batch_size, sequence_length],a_coeffsandb_coeffscan take any shape of[filter_dim, number_of_filters], without being constrained on the input shape.
In this case, when input is a 2D batch of signals, a_coeffs and b_coeffs should be in shape of [batch_size, filter_order + 1] or just [filter_order + 1]. The first one means that the number of filters is equal to batch_size, and each signal is applied with different filter; the second is just one filter apply on all signals.
The case that filter shape will not be constrainted, is when the shape of input is [..., 1, sequence_length]. Then a_coeffs and b_coeffs can be in any shape of 2D matrix [number_of_filters, filter_order + 1], the output shape will be [..., number_of_filters, sequence_length]. It means each signal is filtered by a shared set of filters.
And if I understand correctly, here your test is testing that filter banks produces the same result regardless they are applied separately or together, in that correct?
Yes, that's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I think the batch behavior we want to test is actually the coefficients, not the input. 😆
So we might need to change the test, with a_coeffs and b_coeffs as input batch, waveform as the parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my understanding/expectation is that when input batch is the shape of
[batch_size, sequence_length],a_coeffsandb_coeffscan take any shape of[filter_dim, number_of_filters], without being constrained on the input shape.In this case, when input is a 2D batch of signals,
a_coeffsandb_coeffsshould be in shape of[batch_size, filter_order + 1]or just[filter_order + 1]. The first one means that the number of filters is equal to batch_size, and each signal is applied with different filter; the second is just one filter apply on all signals.The case that filter shape will not be constrainted, is when the shape of input is
[..., 1, sequence_length]. Thena_coeffsandb_coeffscan be in any shape of 2D matrix[number_of_filters, filter_order + 1], the output shape will be[..., number_of_filters, sequence_length]. It means each signal is filtered by a shared set of filters.And if I understand correctly, here your test is testing that filter banks produces the same result regardless they are applied separately or together, in that correct?
Yes, that's correct.
@yoyololicon
Can you help me clarify with the understanding of the shape semantics here?
- When the input batch is 2D, the first dimension is interpreted as channel, thus the number of filters has to match the number of channels.
- When an input signal has multiple channels, then filters have to have multiple channels.
Are these correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me clarify with the understanding of the shape semantics here?
- When the input batch is 2D, the first dimension is interpreted as channel, thus the number of filters has to match the number of channels.
- When an input signal has multiple channels, then filters have to have multiple channels.
Are these correct?
If you want to apply multiple filters at once, these are correct; if there is only one filter, it will fall back to original behavior.
The shape semantics I proposed actually follows pytorch conventions except the last dimension, which is time or filter order.
|
Benchmarks using the same script from #1441 (comment), running on the same cpu. BeforeAfter |
| @parameterized.expand([ | ||
| ((44100,), (2, 3), (2, 44100)), | ||
| ((3, 44100), (1, 3), (3, 44100)), | ||
| ((3, 44100), (3, 3), (3, 44100)), | ||
| ((1, 2, 1, 44100), (3, 3), (1, 2, 3, 44100)) | ||
| ]) | ||
| def test_lfilter_broadcast_shape(self, input_shape, coeff_shape, target_shape): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mthrok
The part that tests the broadcasting behavior.
|
Hi @yoyololicon Sorry for the delayed response. I was busy with release-related work. Now For this PR, I am finding that filter bank support is more complicated than I had imagined. This is natural and I see it analogous to how convolution and broadcasting there is complicated as well. So can we split this PR into one for batch support and the other for filter bank support? I think we can tackle batch support first as the goal is simple, it is just about applying the same operation to a batch of sample without for-loop. Once it's done we can look at the filter bank support to discuss the broadcasting semantics. |
|
@mthrok
I have a different understanding of the batch support feature. I'm ok to split this PR into two, or even three where one for batch, one for filter bank, and the other for broadcasting semantics. I actually think filter bank support is easier than batch support so I suggest to open it first. |
Oh sorry, yes that's what was in my mind. It is not about adding a batch support, but making the batch support efficient (presumably by moving the
Sure. That works. Please proceed with the approach most comfortable for you. I will try my best to understand the underlying logic. |
* updated ddp_pipeline * minor update Co-authored-by: Brian Johnson <[email protected]>
Resolves #1476, resolves #1528.
This implementation doensn't rely on multithreading library or custom cuda kernel, but use only python/C++ API of PyTorch.
After this change,
a_coeffsandb_coeffscan be 1D or 2D Tensor, where the optional dimension means number of filters and should be broadcastable towaveform.shape[:-1].Will do some benchmarks later.