Skip to content

Readable Code for Audio Signal Visualization #2190

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

Merged
merged 4 commits into from
Mar 7, 2023
Merged

Readable Code for Audio Signal Visualization #2190

merged 4 commits into from
Mar 7, 2023

Conversation

lucifertrj
Copy link
Contributor

The following code takes to many variables, and I have simplified the following code into more readable format.

rows = 3
cols = 3
n = rows * cols
fig, axes = plt.subplots(rows, cols, figsize=(16, 9))

for i in range(n):
  if i>=n:
    break
  r = i // cols
  c = i % cols
  ax = axes[r][c]
  ax.plot(example_audio[i].numpy())
  ax.set_yticks(np.arange(-1.2, 1.2, 0.2))
  label = label_names[example_labels[i]]
  ax.set_title(label)
  ax.set_ylim([-1.1,1.1])

to

plt.figure(figsize=(16,10))
for i in range(9):
  plt.subplot(3,3,i+1)
  audio_signal = example_audio.numpy()[i]
  plt.plot(audio_signal)
  plt.title(label_names[example_labels[i]])
  plt.yticks(np.arange(-1.2, 1.2, 0.2))
  plt.ylim([-1.1,1.1])

@github-actions
Copy link

Preview

Preview and run these notebook edits with Google Colab: Rendered notebook diffs available on ReviewNB.com.

Format and style

Use the TensorFlow docs notebook tools to format for consistent source diffs and lint for style:
$ python3 -m pip install -U --user git+https://github.com/tensorflow/docs

$ python3 -m tensorflow_docs.tools.nbfmt notebook.ipynb
$ python3 -m tensorflow_docs.tools.nblint --arg=repo:tensorflow/docs notebook.ipynb
If commits are added to the pull request, synchronize your local branch: git pull origin audio_change

@8bitmp3 8bitmp3 added the review in progress Someone is actively reviewing this PR label Feb 20, 2023
@markmcd
Copy link
Member

markmcd commented Feb 21, 2023

The following code takes to many variables and I have simplified the following code into more readable format.

Thanks - I like your suggestion, but some of these variables are there to improve readability, so I have some requests:

  • Leave the rows, cols and n assignments, and make sure they work. It's consistent with the rest of the document, is clearer, and allows readers to tweak to fit their screen.
  • Leave example_audio[i].numpy() (or just example_audio[i] should work in this context)
  • Add spaces after every comma

@lucifertrj
Copy link
Contributor Author

Hi @markmcd,

Thank you for the review. By code readable, I mainly meant simplifying r = i // cols and c = i % cols with plt.subplot(). I have kept n, rows and cols. And I have made sure that it works. Here is the result:

Screenshot from 2023-02-21 23-36-38

@lucifertrj
Copy link
Contributor Author

Also, no need to mention numpy(). This is included in both the code i.e., official docs and my previous commit. I have fixed it in latest commit.

"cols = 3\n",
"n = rows * cols\n",
"for i in range(n):\n",
" plt.subplot(3, 3, i+1)\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
" plt.subplot(3, 3, i+1)\n",
" plt.subplot(rows, cols, i+1)\n",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry to miss this out. I shall update it

MarkDaoust
MarkDaoust previously approved these changes Feb 21, 2023
@github-actions github-actions bot added the lgtm Community-added approval label Feb 21, 2023
@lucifertrj
Copy link
Contributor Author

@MarkDaoust, I updated (3, 3) to (rows, cols). Sorry, I didn't notice that you already reviewed the PR

@8bitmp3
Copy link
Contributor

8bitmp3 commented Feb 21, 2023

@lucifertrj I formatted the notebook for you again, given the large diff because of the meta data.

TODO for you:

  1. Please remove the cell outputs, and then
  2. Format the notebook again.

This guide here explains how apply the tensorflow-docs nbfmt tool to any IPYNB file to make sure the JSON is cleaner, and passes the tests: https://www.tensorflow.org/community/contribute/docs#notebook_formatting

TL;DR suggested steps:

  1. In a separate Python environment (with venv/virtualenv), or in the same one where you have TensorFlow installed, install the tensorflow-docs package: python3 -m pip install git+https://github.com/tensorflow/docs.
  2. Run: python3 -m tensorflow_docs.tools.nbfmt simple_audio.ipynb

The output should then say:

Format notebook: simple_audio.ipynb

Then you can git add, commit, and push to your branch.

If you have questions, feel free to ask 👍

@lucifertrj
Copy link
Contributor Author

@lucifertrj I formatted the notebook for you again, given the large diff because of the meta data.

TODO for you:

  1. Please remove the cell outputs, and then
  2. Format the notebook again.

This guide here explains how apply the tensorflow-docs nbfmt tool to any IPYNB file to make sure the JSON is cleaner, and passes the tests: https://www.tensorflow.org/community/contribute/docs#notebook_formatting

TL;DR suggested steps:

  1. In a separate Python environment (with venv/virtualenv), or in the same one where you have TensorFlow installed, install the tensorflow-docs package: python3 -m pip install git+https://github.com/tensorflow/docs.
  2. Run: python3 -m tensorflow_docs.tools.nbfmt simple_audio.ipynb

The output should then say:

Format notebook: simple_audio.ipynb

Then you can git add, commit, and push to your branch.

If you have questions, feel free to ask +1

Hi @8bitmp3, I apologise for taking a two days gap. I was busy with my internship work. I will complete the TODO assigned to this PR tomorrow without fail.

@lucifertrj
Copy link
Contributor Author

Done @8bitmp3
Screenshot from 2023-02-25 19-51-53

@lucifertrj
Copy link
Contributor Author

lucifertrj commented Feb 25, 2023

@markmcd @MarkDaoust

The following code does nothing in the simple audio documentation, and train_ds.take(1) doesn't require a break. Because this will return just a single batch/

for example_spectrograms, example_spect_labels in train_spectrogram_ds.take(1):
  break

The above code can be modified into the following:

for example_spectrograms, example_spect_labels in train_spectrogram_ds.take(1):
  print(example_spectrograms.shape)
  print(example_spect_labels.shape)

Should I make the changes, let me know what you think.

@markmcd
Copy link
Member

markmcd commented Feb 27, 2023

The following code does nothing in the simple audio documentation, and train_ds.take(1) doesn't require a break. Because this will return just a single batch/

for example_spectrograms, example_spect_labels in train_spectrogram_ds.take(1):
  break

This code does do something - try removing it, restarting and running the notebook to see what happens. .take(1) returns a dataset, and the for-loop calls its __iter__ method, which materialises rows from the dataset.

You can see in the next cell that example_spectrograms is used. Without this loop, that variable would not exist.

Another way this could be written that might avoid this confusion is:

example_spectrograms, example_spect_labels = next(train_spectrogram_ds.take(1))

This is pretty explicit about what's happening, but next() may be a little harder to grok than a for-loop for someone newer to Python.

@lucifertrj
Copy link
Contributor Author

to grok than a for-loop for someone newer to Python.

Got it thanks

@lucifertrj
Copy link
Contributor Author

@8bitmp3 , the PR is still showing Code Owner Review required. I have made the necessary changes.

@MarkDaoust
Copy link
Member

next(train_spectrogram_ds.take(1))

I think that also needs an iter in there.

@MarkDaoust MarkDaoust removed the review in progress Someone is actively reviewing this PR label Mar 1, 2023
@MarkDaoust MarkDaoust added the ready to pull Start merge process label Mar 1, 2023
@MarkDaoust
Copy link
Member

Thanks. LGTM.

@copybara-service copybara-service bot merged commit 979a26f into tensorflow:master Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Community-added approval ready to pull Start merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants