-
Notifications
You must be signed in to change notification settings - Fork 700
standardizing n_* #298
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
standardizing n_* #298
Conversation
aa4c806
to
cbe41ce
Compare
We should be careful about the distinction between samples (or "time") and frames. The latter is often a small group of samples. |
|
@@ -537,7 +537,7 @@ def lfilter(waveform, a_coeffs, b_coeffs): | |||
Performs an IIR filter by evaluating difference equation. | |||
|
|||
Args: | |||
waveform (torch.Tensor): audio waveform of dimension of `(n_channel, n_frames)`. Must be normalized to -1 to 1. | |||
waveform (torch.Tensor): audio waveform of dimension of `(channel, time)`. Must be normalized to -1 to 1. |
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.
Should this be `frame'.
@@ -558,37 +557,37 @@ def lfilter(waveform, a_coeffs, b_coeffs): | |||
|
|||
device = waveform.device | |||
dtype = waveform.dtype | |||
n_channels, n_frames = waveform.size() | |||
n_channel, n_sample = waveform.size() |
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.
For consistency, channel
and frame
, rather than n_channel
and n_sample
?
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.
We use n_*
to refer to the actual value, and *
to the name of the dimension. Based on this convention, this would be ok. We could decide to change the convention itself, but I'd leave that to a separate PR if at all.
n_order = a_coeffs.size(0) | ||
assert(n_order > 0) | ||
|
||
# Pad the input and create output | ||
padded_waveform = torch.zeros(n_channels, n_frames + n_order - 1, dtype=dtype, device=device) | ||
padded_waveform = torch.zeros(n_channel, n_sample + n_order - 1, dtype=dtype, device=device) |
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.
Same as above.
padded_waveform[:, (n_order - 1):] = waveform | ||
padded_output_waveform = torch.zeros(n_channels, n_frames + n_order - 1, dtype=dtype, device=device) | ||
padded_output_waveform = torch.zeros(n_channel, n_sample + n_order - 1, dtype=dtype, device=device) |
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.
Same as above.
a0_repeated = torch.ones(n_channels, dtype=dtype, device=device) * a_coeffs[0] | ||
ones = torch.ones(n_channels, n_frames, dtype=dtype, device=device) | ||
a0_repeated = torch.ones(n_channel, dtype=dtype, device=device) * a_coeffs[0] | ||
ones = torch.ones(n_channel, n_sample, dtype=dtype, device=device) |
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 saw frame
, time
, sample
are used. Do they refer to the same thing?
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.
Before this PR, time
was used to indicate time dimension, and n_frames
and n_samples
were used inconsistently to refer to the number of time sample available. Here, I'm suggesting to use frame
(and n_frame
) to refer to "groups of samples" (e.g. in STFT, we have rolling windows of samples to operate on), and "n_sample" to the number of samples in a "time" dimension.
We could also decide to change "time" dimension to "sample" dimension, but that does not seem to have been a source of confusion, so I would avoid changing this in this PR.
We want to clarify the naming of
n_*
variables, initiated by #293. Documentation of standardization in README.