NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

changed default values of Axes.specgram()#9221

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

andreh7 wants to merge1 commit intomatplotlib:masterfromandreh7:2017-08-specgram-docstring-default-values

Closed

changed default values of Axes.specgram()#9221

andreh7 wants to merge1 commit intomatplotlib:masterfromandreh7:2017-08-specgram-docstring-default-values

Conversation

Copy link

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

changed default values of Axes.specgram() as proposed inmatplotlib#9…

81cdc04

…100 (comment)

andreh7 mentioned this pull request

Sep 25, 2017

Parameter Fs initialization missing in Axes.specgram()#9100

Closed

dstansby approved these changes

Sep 25, 2017

View reviewed changes

Copy link

Member

dstansby left a comment

Choose a reason for hiding this comment

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

👍

Copy link

Member

I still think this is the wrong way to go, and thataxes.specgram should passNone arguments through tomlab.specgram so the defaults inmlab.specgram are used. Otherwise, the two are in danger of becoming inconsistent.

To properly format the plot, theoutput frommlab.specgram should be used, not the input. Given that the plotting function only usesFs, andFs is easily retrievable from the output (= 2 * freq[-1]) this is easily done.

Copy link

Member

dstansby commentedSep 25, 2017

(FWIW I think eventuallymlab.specgram should disappear and be replaced byhttps://docs.scipy.org/doc/scipy-0.17.0/reference/generated/scipy.signal.spectrogram.html sinceMatplotlib is really a plotting library and not a spectral analysis library)

Copy link

Contributor

anntzer commentedNov 28, 2017

pyplot would need to be regenerated too (or#9173 merged :-)). And I agree that the whole thing should be deprecated anyways.

Copy link

Member

jklymak commentedApr 24, 2018

I'm going to close this, because its sat for quite a while and I don't think the proposed behaviour is correct. Feel free to re-open if I am mistaken.