Skip to content

[WIP] Adding functionality for tfr_array_stft and tfr_stft#8892

Closed
adam2392 wants to merge 4 commits intomne-tools:mainfrom
adam2392:stft
Closed

[WIP] Adding functionality for tfr_array_stft and tfr_stft#8892
adam2392 wants to merge 4 commits intomne-tools:mainfrom
adam2392:stft

Conversation

@adam2392
Copy link
Copy Markdown
Member

Reference issue

Closes: #8880

Need to implement unit tests.

What does this implement/fix?

  • Implements tfr_array_stft and tfr_stft which operate on numpy arrays and mne-python objects respectively.

  • Deprecates stft in favor of tfr_array_stft.

Additional information

No parallelizations across channels because felt that could be saved for a later PR.

@agramfort
Copy link
Copy Markdown
Member

thinking about it we have an STFT from the TF-MxNE solver. This stft implementation uses
a certain convention for edge which is ok for TF-MxNE but maybe non-standard. I would
maybe suggest to use the scipy implementation these new public functions and then make the
current stft function private and just used Tf-MxNE.

what do you think @larsoner ?

@larsoner
Copy link
Copy Markdown
Member

I think eventually we want to replace the TF-MxNE version anyway, so I don't think there is a big difference. One advantage of our version is that it preserves power properly, whereas the SciPy version doesn't (it does on round trip, just not between time and time-frequency representations), there is an issue about it:

scipy/scipy#13489

So I actually don't mind using our internal version to start and then hopefully eventually just changing its internal calls to use stft.

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Feb 25, 2021 via email

@larsoner
Copy link
Copy Markdown
Member

if the scipy code much faster? if so I would really encourage to use this one.

I haven't checked but, having looked at your code, I wouldn't expect there to be large differences between the two

@adam2392
Copy link
Copy Markdown
Member Author

Should I leave things as is and add a note to change things once scipy implementation has been fixed?

Copy link
Copy Markdown
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

I would really encourage you @adam2392 to do a benchmark with the scipy code in terms of speed and result quality.


for epochidx in range(n_epochs):
# compute STFT
power = _stft(data[epochidx, ...], window_size=window_size,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I doubt the stft returns power by default. It should output complex valued data. Can you check?

also I would see if I can preallocate the power_list as a numpy array before the for loop to avoid doubling the RAM usage.


@fill_doc
def tfr_stft(inst, window_size, step_size=None, average=True, verbose=None):
"""Compute STFT Short-Term Fourier Transform using a sine window.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you meant Short-Time Fourier Transform here.

@adam2392 adam2392 closed this Aug 25, 2021
@adam2392 adam2392 deleted the stft branch August 25, 2021 01:31
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.

Implementation of stft() function as stft_array and tfr_stft

4 participants