Parameter Fs initialization missing in Axes.specgram()

[andreh7]

Bug report

Bug summary

the docstring of parameter Fs of matplotlib.pyplot.specgram() says that the default value is 2 but the default value in the function signature is None.

Code for reproduction

point your browser to https://matplotlib.org/devdocs/api/_as_gen/matplotlib.pyplot.specgram.html or do:

import matplotlib.pyplot as plt
help(plt.specgram)

Actual outcome

specgram(x, NFFT=None, Fs=None, Fc=None, ...
...
Fs : scalar
        The sampling frequency (samples per time unit).  It is used
        to calculate the Fourier frequencies, freqs, in cycles per time
        unit. The default value is 2.

Expected outcome

Either write None in the docstring or make change the default value of Fs to 2.

Matplotlib version

• Operating System: OSX 10.11.6
• Matplotlib Version: 2.0.2
• Python Version: 3.5.4
• Jupyter Version (if applicable): notebook server 5.0.0, IPython 6.1.0
• Other Libraries: N/A

Installed with pip in a conda environment.

[tacaswell]

I do not think this is a problem, the default value used is 2.

specgram just passes Fs down into a helper function (which is where the 2 is finally hard-coded). I do not think we do want put the default value into every signature of that calls the helper (as we would then have the default value encoded a bunch of places instead of just one).

I am against changing the docstring to say it defaults to None as the value needs to be an integer at some point to turn the crank on the math and we use the value 2.

[andreh7]

When I do:

import matplotlib.pyplot as plt
import numpy as np

plt.specgram(np.ones(128))

I get an exception:

...
lib/python3.5/site-packages/matplotlib/axes/_axes.py in specgram(self, x, NFFT, Fs, Fc, detrend, window, noverlap, cmap, xextent, pad_to, sides, scale_by_freq, mode, scale, vmin, vmax, **kwargs)
   7243         if xextent is None:
   7244             # padding is needed for first and last segment:
-> 7245             pad_xextent = (NFFT-noverlap) / Fs / 2
   7246             xextent = np.min(t) - pad_xextent, np.max(t) + pad_xextent
   7247         xmin, xmax = xextent

TypeError: unsupported operand type(s) for /: 'int' and 'NoneType'

while when I do:

plt.specgram(np.ones(128), Fs = 2)

I get a spectrogram graph.

So the effective default value is not 2.

[dstansby]

Very early on in the specgram code is:

        if NFFT is None:
            NFFT = 256  # same default as in mlab.specgram()
        if Fc is None:
            Fc = 0  # same default as in mlab._spectral_helper()
        if noverlap is None:
            noverlap = 128  # same default as in mlab.specgram()

which misses out setting Fs = 2. (Also this means that the defaults are hardcoded anyway, so having default kwargs instead of all these if statements seems more sensible to me)

[tacaswell]

Ah, sorry I was looking at mlab.specgram not Axes.specgram 🐑 .

👍 to moving the default values up into the signature of Axes.specgram (and regenerating pyplot) for as many of the kwargs as make sense.

[tacaswell]

@andreh7 Want to take a shot at this?

[tacaswell]

I would go with everything listed in

          specgram(x, NFFT=256, Fs=2, Fc=0, detrend=mlab.detrend_none,
                   window=mlab.window_hanning, noverlap=128,
                   cmap=None, xextent=None, pad_to=None, sides='default',
                   scale_by_freq=None, mode='default', scale='default',
                   **kwargs)

Exact work:

• update the axes/_axes:Axes.specgram method signature to match the above
• leave the None handling (to not break people who are explicitly passing in None to mean 'do the default thing'
• add handling for Fs=None to default to 2 to fix the bug
• add a test with Fs=None to the test suite (does not need to be an image test)

[andreh7]

sorry for the late reply, I'm looking into this now. What about the vmin and vmax parameters in the signature you proposed for axes/_axes:Axes.specgram in #9100 (comment) ? These parameters appear in the code (currently they default to None).

I also see that (at least in the current head d02024e), mlab.specgram() would then have different default values than the ones proposed in the above comment. Should these be changed as well ?

[tacaswell]

@andreh7 Can you open a PR with that commit?

[jklymak]

I'd suggest that ax.specgram() should be changed to:

     pad_xextent = (NFFT-noverlap) / freq[-1] / 2

...that way you aren't updating defaults in two places.

I find this call strange anyway. I'd have thought:
xmin = t[0]-(t[1]-t[0])/2. and xmax = t[-1]+(t[-2]-t[-1])/2. would be more straightforward. But I haven't tested.

[andreh7]

@tacaswell I created pr #9221

What about the default values in mlab.specgram() (see my question in #9100 (comment) ) ?