Skip to content

[WIP] - Add / use / update compute_spectrogram#359

Open
TomDonoghue wants to merge 4 commits intomainfrom
spect
Open

[WIP] - Add / use / update compute_spectrogram#359
TomDonoghue wants to merge 4 commits intomainfrom
spect

Conversation

@TomDonoghue
Copy link
Member

@TomDonoghue TomDonoghue commented Oct 29, 2025

This PR relates to the note about the spectrogram implementation in #344 - and also relates in part to discussion & updates in #357 and #358. Notably, while the #357 update moved away from using spectrogram in our Welch's function, this doesn't fully address the note about the implementation change in #344, since we use the scipy.signal.spectrogram function elsewhere.

To start with, this PR:

  • adds an explicit compute_spectrogram function to unify and make explicit our use of spectrograms (this initial version uses scipy.signal.spectrogram, so no change in behavior)
  • updates to use compute_spectrogram in relevant places in neurodsp.spectral.variance.

From here, the question is which implementation of spectrogram to support. We can add (instead or in addition), the new scipy.signal.ShortTimeFFT.spectrogram implementation.

From here, we could update to use scipy's new implementation of spectrogram, and it would look something like this:

from scipy.signal import ShortTimeFFT
from neurodsp.spectral.checks import check_windowing_settings

def compute_spectrogram_new(sig, fs, window='hann', nperseg=None, noverlap=0, nfft=None):
    
    nperseg, noverlap = check_windowing_settings(fs, window, nperseg, noverlap)    

    stfft = ShortTimeFFT.from_window(window, fs=fs, nperseg=nperseg, noverlap=noverlap, mfft=nfft,
                                     fft_mode='onesided', scale_to='psd', phase_shift=None)
    spg = stfft.spectrogram(sig, detr='constant')
    
    return stfft.f, stfft.t(len(sig)), spg

However, there are some differences in the windowing and defaults that seem a bit nitpicky to solve. We would need to decide if / to what extent to match behavior between new & old (alternately: to what extent to just take the new behavior).

Relevant notes on aligning the implementations:

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.33%. Comparing base (f5b7b69) to head (a069fef).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #359   +/-   ##
=======================================
  Coverage   98.32%   98.33%           
=======================================
  Files         119      119           
  Lines        4430     4435    +5     
=======================================
+ Hits         4356     4361    +5     
  Misses         74       74           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant