-
Notifications
You must be signed in to change notification settings - Fork 739
feat: lfilter with batch support #1638
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 proposal. Please correct me if my understanding is wrong. In this use case, do you want to apply a batch of filters to a single waveform as a batch training example? Or is the batch_size of the training example as 1 and |
|
@yoyololicon Thanks for the suggestion. In your description, can you replace the The way I think of "batching" (of samples) in PyTorch or other DL framework is "processing multiple sample data points at the same time for the sake of computational efficiency". And, the resulting data points should be same regardless of whether they were batched or not. (or batched in a different order.) From this view point, the first one seems to violate the independency.
If different filter is applied to different samples, if the input batch is shuffled along the batch dimension, then the result will be different before/after shuffling. And I think this "Applying each filter to each sample individually" is somewhat analogous to the group convolution, and if we want to support this kind of "group"-ing, then I think we should introduce a notion of input group dimension. (like a channel) So the way I think of batch/bank shpae semantics so far is as follow. I think this can handle the cases where batch dimension or channel dimension is 1, or where both batch and channel dimension present. 1. no bank (coeffs is 1D)1.1. without batch dim1.2. with batch dimThe same filter is applied independently to each sample along batch dimension 1.3. with batch and possibly more dimsThe same filter is applied independently to each dim of each sample. Similar to 2. With bank (coeff is 2D), without grouping (say,
|
|
@nateanl @mthrok Re-write section 2 examples from above base on this idea: 2. With bank (coeff is 2D), without groupingIf the waveform is over 1D, user should manually unsqueeze the second from last axis before passing the waveform. 2.1. without batch dimI actually think this case is valid though, and the current 2.2. with batch dim2.3. with batch and possibly more dims3. With bank and grouping3.1. without batch3.2. with batch and possibly more dims |
|
@yoyololicon
So regarding the
|
@mthrok I suggest the parameter |
Sure, I am okay with boolean. Also we can think of a better naming than |
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.
HI @yoyololicon
Thanks for the PRs. Please let me know your thoughts on my comments.
torchaudio/functional/filtering.py
Outdated
| batching (bool, optional): Activate when coefficients are in 2D. If ``True``, then waveform should be at least | ||
| 2D, and the size of second axis from last should equals to ``num_filters``. | ||
| The output can be expressed as ``output[..., i, :] = lfilter(waveform[..., i, :], | ||
| a_coeffs[i], b_coeffs[i], clamp=clamp, batching=False)``. (Default: ``False``) |
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.
The output can be expressed as
output[..., i, :] = lfilter(waveform[..., i, :], a_coeffs[i], b_coeffs[i], clamp=clamp, batching=False)
Can you remind the necessity to have interweaving behavior, when the above can achieve the same result?
I guess computational efficiency is one, and autograd support is the second reason?
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.
The main reason is computational efficiency, this remove an extra for-loop. I can benchmark this feature if needed.
Both way support autograd though.
| assert waveform.ndim > 1 | ||
| assert waveform.shape[-2] == a_coeffs.shape[0] | ||
| else: | ||
| waveform = torch.stack([waveform] * a_coeffs.shape[0], -2) |
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.
This shape manipulation looks very complicated, and I wonder if it is because we have pack/unpack batch in the following section. If so, is there a way to contain pack/unpack batch bellow into batching mechanism?
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.
I think it's not related to pack/unpack batch, and I haven't come up a way to integrate these two parts together.
Waveform and coefficients should have equal number of channels before pack batch, and this line is just replicating the waveform to match the desire shape.
| Lower delays coefficients are first, e.g. ``[b0, b1, b2, ...]``. | ||
| Must be same size as a_coeffs (pad with 0's as necessary). | ||
| clamp (bool, optional): If ``True``, clamp the output signal to be in the range [-1, 1] (Default: ``True``) | ||
| batching (bool, optional): Activate when coefficients are in 2D. If ``True``, then waveform should be at least |
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.
| batching (bool, optional): Activate when coefficients are in 2D. If ``True``, then waveform should be at least | |
| batching (bool, optional): Effective only when coefficients are 2D. If ``True``, then waveform should be at least |
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.
Thanks,. Should I make another MR to fix the docstring?
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.
If you have time, please!
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.
LGTM. Thanks!
…h#1638) * tensorqs_tutorial -> tensors_tutorial * autogradqs_tutorial -> autograds_tutorial * spaces * fix link to torch.Tensor.scatter_ documentation * undo file renaming * autogradqs_tutorial.py in readme Co-authored-by: Holly Sweeney <[email protected]>
Resolves #1476, relates to #1561.
@mthrok
I want to take some discusssions about the design before dive into it.
The batching feature I have imagined, is when input is a multi-dimensional tensor with shape
(..., bank_size, ..., time), and filter coefficients are 2D matrices with shape(bank_size, filter_order). The filters are applied individually at thebatchfilterbanks dimension, so the output shape is also(..., bank_size, ..., time).In PyTorch convention, the first dimension are usually considered as batch dimension, so the input shape should be(bank_size, ..., time). To incoporate along with filter banks feature, maybe we should add a boolean parameter to enable it so the output won't become(bank_size, ..., bank_size, time)?Actually, my original design from #1561, is putting the
batchfilterbanks dimension at the second from last axis(..., bank_size, time). To enable filter banks support but not batching support, user can unsqueeze the second from last axis, so the input shape is(..., 1, time), and the output will be(..., bank_size, time)if filter coefficients are(bank_size, filter_order). In this way, no extra parameter is needed.To state it more clearly:
batching
filterbanks
Any ideas?