Skip to content

Conversation

@jamarshon
Copy link
Contributor

@jamarshon jamarshon commented Jul 25, 2019

-With new features added and legacy code gets removed, the docs (rst) also have to be updated
-The current docs looks very shady/broken (some code missing e.g transforms forward) or formatted poorly
Preview: https://jamarshon.github.io/audio/

@jamarshon jamarshon changed the title [WIP] Large reamp on the torchaudio/docs Large reamp on the torchaudio/docs Jul 25, 2019
@jamarshon jamarshon mentioned this pull request Jul 25, 2019
@cpuhrsch cpuhrsch changed the title Large reamp on the torchaudio/docs Large revamp on the torchaudio/docs Jul 26, 2019
@vincentqb
Copy link
Contributor

vincentqb commented Jul 26, 2019

MelScale renders a little strange here.

@vincentqb
Copy link
Contributor

vincentqb commented Jul 26, 2019

MelSpectrogram has inconsistent ":" in subtitles, see here.

@jamarshon jamarshon changed the title Large revamp on the torchaudio/docs Large re-amp on the torchaudio/docs Jul 26, 2019
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that we should add: just like in #169, we should give the mapping between shapes for transformations.

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for doing this!

@vincentqb
Copy link
Contributor

vincentqb commented Jul 26, 2019

On Spectrogram.forward, the shape is listed as "Channels x frequency x time (c, f, t)". I would stick with either convention, but not both. I would lean for "a tensor of shape (channels, frequency, time)", and not abbreviations to as explicit as possible for both input and output of forward.

@vincentqb
Copy link
Contributor

Do we settle "(channel, ...)" or "(channels, ...)" ? :) I'm leaning for singular, since all other words are singular. Thoughts?

@vincentqb
Copy link
Contributor

Similarly, do we settle on "a tensor of size (...)" or "a tensor of shape (...)"? That's one of those size/shape/dimension discussions :)

@vincentqb
Copy link
Contributor

Do we settle "(channel, ...)" or "(channels, ...)" ? :) I'm leaning for singular, since all other words are singular. Thoughts?

Also, I noticed we had agreed on n_freqs to refer to frequency, so we should update that in the documentation. But then this is a little inconsistent with time and channel. Should we have n_times and n_channels or leave it? Thoughts?

@vincentqb
Copy link
Contributor

Let's use the convention described in #169 for the comments quoted below now that we've settled on it.

Do we settle "(channel, ...)" or "(channels, ...)" ? :) I'm leaning for singular, since all other words are singular. Thoughts?

channel, and singular words

Similarly, do we settle on "a tensor of size (...)" or "a tensor of shape (...)"? That's one of those size/shape/dimension discussions :)

dimensions (for "dimension names") and size for sizes (we'll avoid shape as much as we can)

Also, I noticed we had agreed on n_freqs to refer to frequency, so we should update that in the documentation. But then this is a little inconsistent with time and channel. Should we have n_times and n_channels or leave it? Thoughts?

n_freq, n_channel, n_time, etc. for sizes

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about #169

@cpuhrsch cpuhrsch merged commit 95235f3 into pytorch:master Jul 29, 2019
@jamarshon jamarshon deleted the docs branch July 29, 2019 16:31
jamarshon added a commit that referenced this pull request Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants