FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix layout of spectrum_demo.py by DietBru · Pull Request #8158 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fix layout of spectrum_demo.py - #8158

Merged
NelleV merged 3 commits into
matplotlib:masterfrom
DietBru:spectrum_demo
Mar 6, 2017
ConversationCommits (3)ChecksFiles changed
Merged

Fix layout of spectrum_demo.py#8158
NelleV merged 3 commits into
matplotlib:masterfrom
DietBru:spectrum_demo

Conversation

DietBru commented Feb 26, 2017

Copy link
Copy Markdown
Contributor

Fixes eighth entry in issue #7956

anntzer commented Feb 27, 2017

Copy link
Copy Markdown
Contributor
  • Any reason why you swapped the angle and phase plots?
  • I would remove the $t$ and $s(t)$ unless you want to add similar labels for all subplots.

phobson requested changes Feb 27, 2017
View reviewed changes
Comment thread examples/pylab_examples/spectrum_demo.py Outdated

plt.subplot(3, 2, 4)
plt.magnitude_spectrum(s, Fs=Fs, scale='dB')
fig, axx = plt.subplots(3, 2)

phobson Feb 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I see what you're doing here. But I think we should strive for consistency in the examples/gallery.

In my mind that means, for a single axes object:

fig, ax = plt.subplots()

And for multiples axes:

fig, axes = plt.subplots(ncols=C, nrows=R)

Comment thread examples/pylab_examples/spectrum_demo.py Outdated
# plot different spectrum types:
axx[1, 0].magnitude_spectrum(s, Fs=Fs)
axx[2, 0].phase_spectrum(s, Fs=Fs)
axx[0, 1].remove() # don't display empty ax

phobson Feb 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

perhaps move this to the end.

DietBru Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good points - fixed them.

DietBru commented Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

@anntzer I swapped the plots around for aesthetic reasons: The minus signs in the y-axis changes the position of the y-labels. $t$ and $s(t)$ are removed.
Furthermore, I would argue that for a correct energy the scaling of the spectrum is of by a factor of 1/Fs or so. I'll investigate that a bit more and put that into a different pull request.

tacaswell reviewed Mar 1, 2017
View reviewed changes
Comment thread examples/pylab_examples/spectrum_demo.py

plt.subplot(3, 2, 6)
plt.phase_spectrum(s, Fs=Fs)
# plot different spectrum types:

tacaswell Mar 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

might be worth giving them titles / axis labels?

phobson Mar 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@vollbier sorry to move the goal post on you.

In addition to Tom's request could you:

  1. make the figure bigger (add figsize=(7, 7) to the call to subplots)
    2) make the Magnitude y-axis log-scaled (if that's appropriate for the domain)

DietBru Mar 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I added titles, but I think it's worthwhile leaving the default axis labels, since it's feature of the spectrum functions.

phobson requested changes Mar 1, 2017
View reviewed changes
Comment thread examples/pylab_examples/spectrum_demo.py

plt.subplot(3, 2, 6)
plt.phase_spectrum(s, Fs=Fs)
# plot different spectrum types:

phobson Mar 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@vollbier sorry to move the goal post on you.

In addition to Tom's request could you:

  1. make the figure bigger (add figsize=(7, 7) to the call to subplots)
    2) make the Magnitude y-axis log-scaled (if that's appropriate for the domain)

phobson changed the title Fix layout of spectrum_demo.py [MRG+1] Fix layout of spectrum_demo.py Mar 3, 2017
NelleV merged commit a8547fa into matplotlib:master Mar 6, 2017

NelleV commented Mar 6, 2017

Copy link
Copy Markdown
Member

Thanks @vollbier ! This is so much better!

DietBru deleted the spectrum_demo branch March 6, 2017 21:26
anntzer mentioned this pull request Mar 6, 2017
Closed
16 tasks done
QuLogic changed the title [MRG+1] Fix layout of spectrum_demo.py Fix layout of spectrum_demo.py Mar 6, 2017
QuLogic added this to the 2.1 (next point release) milestone Mar 6, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments
phobson phobson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL