Skip to content
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

Update audio resampling tutorial #2386

Closed
wants to merge 1 commit into from

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented May 12, 2022

  • Replace mis-use of plot_specgram with plot_sweep, and remove plot_specgram
  • Move benchmark_resample to later section

https://output.circle-artifacts.com/output/job/9f7af187-777d-4d75-840f-2630a36295b7/artifacts/0/docs/tutorials/audio_resampling_tutorial.html

@mthrok mthrok force-pushed the update-audio-resample-tutorial branch from 2b48b19 to 95f4fbb Compare May 12, 2022 04:09
@mthrok mthrok marked this pull request as ready for review May 16, 2022 04:20
@facebook-github-bot
Copy link
Contributor

@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

# -------------------------------------------------------------------------------
# Preparation of data and helper functions.
# -------------------------------------------------------------------------------
# Firstly, we import the modules and define the helper functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Firstly, we import the modules and define the helper functions.
# First, we import the modules and define the helper functions.

#
# Lowpass filter width
# ~~~~~~~~~~~~~~~~~~~~
# 3.1. Lowpass filter width
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my comment in data augmentations PR: I prefer subsections in indented form over subsection numbers. I think this organization is a bit harder to read and also places the same amount of emphasis on the subsections as the main sections

Screen Shot 2022-05-16 at 10 15 39 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is intentional change. The previous way, one has to click these subsection so many times par tutorial, and the fact that state of the collapse/uncollapse does not persist across refreshing (also, from time to time, it renders wrong after refresh where it is supposed to be uncollapsed) is very bad UX. Also uncollapsing one subsection moves the button for uncollapsing the subsequent subsections. This is common UX issue across the documentation. (much worse on functional/transform page, considering the number of items listed there)

@mthrok mthrok force-pushed the update-audio-resample-tutorial branch 2 times, most recently from 7a4c218 to b457af6 Compare May 22, 2022 04:08
@mthrok mthrok added this to the v0.12 milestone May 24, 2022
- Replace mis-use of plot_specgram with plot_sweep, and remove plot_specgram
- Move `benchmark_resample` to later section
- Add numbers to section title
@mthrok mthrok force-pushed the update-audio-resample-tutorial branch from b457af6 to 4c61c7f Compare June 2, 2022 04:02
Copy link
Contributor

@carolineechen carolineechen left a comment

Choose a reason for hiding this comment

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

changes look good! could you update the PR description?

@facebook-github-bot
Copy link
Contributor

@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

Hey @mthrok.
You merged this PR, but labels were not properly added. Please add a primary and secondary label (See https://github.com/pytorch/audio/blob/main/.github/process_commit.py)

@mthrok mthrok deleted the update-audio-resample-tutorial branch June 3, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants