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

Fix layout of spectrum_demo.py #8158

Merged
merged 3 commits into from Mar 6, 2017
Merged

Fix layout of spectrum_demo.py #8158

merged 3 commits into from Mar 6, 2017

Conversation

DietBru
Copy link
Contributor

@DietBru DietBru commented Feb 26, 2017

Fixes eighth entry in issue #7956

@anntzer
Copy link
Contributor

anntzer commented Feb 27, 2017

  • 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.


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

Choose a reason for hiding this comment

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

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)

# 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
Copy link
Member

Choose a reason for hiding this comment

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

perhaps move this to the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points - fixed them.

@DietBru
Copy link
Contributor Author

DietBru commented Feb 28, 2017

@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.


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

Choose a reason for hiding this comment

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

might be worth giving them titles / axis labels?

Copy link
Member

@phobson phobson Mar 1, 2017

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

@phobson phobson Mar 1, 2017

Choose a reason for hiding this comment

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

@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 phobson changed the title Fix layout of spectrum_demo.py [MRG+1] Fix layout of spectrum_demo.py Mar 3, 2017
@NelleV NelleV merged commit a8547fa into matplotlib:master Mar 6, 2017
@NelleV
Copy link
Member

NelleV commented Mar 6, 2017

Thanks @vollbier ! This is so much better!

@DietBru DietBru deleted the spectrum_demo branch March 6, 2017 21:26
@anntzer anntzer mentioned this pull request Mar 6, 2017
16 tasks
@QuLogic QuLogic changed the title [MRG+1] Fix layout of spectrum_demo.py Fix layout of spectrum_demo.py Mar 6, 2017
@QuLogic QuLogic added this to the 2.1 (next point release) milestone Mar 6, 2017
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.

None yet

6 participants