MTG / sms-tools

Sound analysis/synthesis tools for music applications
https://www.upf.edu/web/mtg/sms-tools
GNU Affero General Public License v3.0
1.64k stars 754 forks source link

scipy.signal.get_window(window,M,fftbins=True) takes a 3rd argument #78

Open vichug opened 6 years ago

vichug commented 6 years ago

...the fftbins=True argument (True beeing the default value), which is not mentionned throughout of the course, and can lead to precision errors easily in a multitude of cases, because it forces the window to be periodic or symmetric* :

fftbins : bool, optional
    If True (default), create a "periodic" window, ready to use with
    `ifftshift` and be multiplied by the result of an FFT (see also
    `fftpack.fftfreq`).
    If False, create a "symmetric" window, for use in filter design.

So if this argument is unchanged, get_window will always return a periodic window. This option could possibly have been added in a modern-ish version of scipy, hence why it's not beeing adressed. (see here https://www.coursera.org/learn/audio-signal-processing/programming/AZcF0/short-time-fourier-transform-stft/discussions/threads/W6_BoR4dEeiRQwooVK8Q3A)

*a first wrong understanding on my end was that this parameter changed the length of the window to be of an even or odd size, and change the length in samples of the output. It doesn't seem to change the length of the output. The way i understand it, if for example the size in samples is odd and the window supposed to be even-sized, then it may create an even sized window and zero-pad the last sample or the first one. In any case, the difference is real. See https://en.wikipedia.org/wiki/Window_function#Symmetry . I could add this line to have get_window behave consistently with what was expected from it : fftbins = M%2==0 ; M beeing the window size ; then passing fftbins argument to get_window.

This is possibly related to this SciPy commit & discussion : https://github.com/scipy/scipy/pull/6483

DoomyDwyer commented 4 years ago

From my experiments, this only appears to have an effect on the phase spectrum.

zjuPeco commented 4 years ago

...the fftbins=True argument (True beeing the default value), which is not mentionned throughout of the course, and can lead to precision errors easily in a multitude of cases, because it forces the window to be periodic or symmetric* :

fftbins : bool, optional
    If True (default), create a "periodic" window, ready to use with
    `ifftshift` and be multiplied by the result of an FFT (see also
    `fftpack.fftfreq`).
    If False, create a "symmetric" window, for use in filter design.

So if this argument is unchanged, get_window will always return a periodic window. This option could possibly have been added in a modern-ish version of scipy, hence why it's not beeing adressed. (see here https://www.coursera.org/learn/audio-signal-processing/programming/AZcF0/short-time-fourier-transform-stft/discussions/threads/W6_BoR4dEeiRQwooVK8Q3A)

*a first wrong understanding on my end was that this parameter changed the length of the window to be of an even or odd size, and change the length in samples of the output. It doesn't seem to change the length of the output. The way i understand it, if for example the size in samples is odd and the window supposed to be even-sized, then it may create an even sized window and zero-pad the last sample or the first one. In any case, the difference is real. See https://en.wikipedia.org/wiki/Window_function#Symmetry . I could add this line to have get_window behave consistently with what was expected from it : fftbins = M%2==0 ; M beeing the window size ; then passing fftbins argument to get_window.

This is possibly related to this SciPy commit & discussion : scipy/scipy#6483

You are my life-saver!!!

awagenknecht commented 5 months ago

Commenting because this was still a pain point when I took the course in 2023, and it looks like it hasn't been addressed. I loved the class and want to help maintain it if I can!

Prior to SciPy 0.18, an odd window length would always return a symmetric window, even though the default is for a periodic window (fftbins=True). SciPy 0.18 fixed this so that an odd-length window could be either periodic or symmetric as expected, depending on the fftbins parameter. The problem is that the Coursera grader still expects answers where get_window would have returned an odd-length symmetric window. Since the lectures encourage students to use odd-length windows and don't mention the fftbins parameter at all, most students end up with the default periodic windows and differences from the "correct" answer that are enough to get marked wrong.

Setting fftbins = M%2==0 throughout the code would be setting consistency with an outdated SciPy version, and it doesn't account for what students write in their assignments. It seems like the better solution would be to redo the answer key using fftbins=True periodic windows for everything so that students can use the default, but I don't have any idea if redoing the answer key is a possibility.