Update and move convention section to CONTRIBUTING.md #1635
Conversation
yangarbiter
force-pushed
the
fix_convention_doc
branch
2 times, most recently
from
89de503 to
e3a6636
Compare
CONTRIBUTING.md
Outdated
| * `win_length`: the length of the STFT window | ||
| * `window_fn`: for functions that creates windows e.g. `torch.hann_window` | ||
|
|
||
| Transforms expect and return the following dimensions. |
There was a problem hiding this comment.
Let's remove the transforms. This is not a convention. The same thing is explained in the doc, and there should be only one source of truth. Also this is not necessarily relevant to new contributors.
There was a problem hiding this comment.
I agree with this. If I need to know the input or output, I would just go to the documentation website instead of here. I've removed them in the latest commit.
| * `f_min`: the lowest frequency of the lowest band in a spectrogram | ||
| * `f_max`: the highest frequency of the highest band in a spectrogram | ||
| * `win_length`: the length of the STFT window | ||
| * `window_fn`: for functions that creates windows e.g. `torch.hann_window` |
There was a problem hiding this comment.
Looking at the code, the explanation, and your comment I think only waveform, and sample_rate is standard and relevant across the code base. If there are only one or two usage, then it's not really convention or standard. And when a new contributor add a new feature, and when we review it, as a good software development practice, we naturally try to stick to existing variable names. So I doubt we need this.
Rather, this list of convention is rather confusing and limiting because IIRC there are cases where waveform have batch dimension in addition to channel and time, which seems like not following the convention.
How about removing this?
There was a problem hiding this comment.
I actually think we should keep it. I think this short table can help for a quick reference. I've updated the batch dimension problem and rewrite this paragraph a bit.
Please let me know how you think about this new version.
Thanks.
yangarbiter
force-pushed
the
fix_convention_doc
branch
from
e3a6636 to
e236dd7
Compare
yangarbiter
force-pushed
the
fix_convention_doc
branch
2 times, most recently
from
cf98c56 to
15fffe3
Compare
yangarbiter
force-pushed
the
fix_convention_doc
branch
from
15fffe3 to
2796d2b
Compare
yangarbiter
changed the title
n_melton_melssince all related variables in functional and (transforms)[https://github.com/pytorch/audio/blob/master/torchaudio/transforms.py] are calledn_mels.n_freq, there are two exceptions (1 and 2) calling itn_freqs.min_freqandmax_freqare changed tof_minandf_maxare used here and no other function usesmin_freqormax_freq.