Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time-frequency in Source Space #6629

Closed
wants to merge 101 commits into from
Closed

Conversation

DiGyt
Copy link
Contributor

@DiGyt DiGyt commented Aug 5, 2019

Reference issue

This is the main PR for the GSoC TFR in Source Space Project (See: Issue #6290 ).

What does this implement/fix?

This PR will allow SourceEstimates or lists/generators of SourceEstimates to be passed into time_frequency functions. This means that the functions tfr_morlet, tfr_multitaper and tfr_stockwell will accept all types of SourceEstimates, and compute the correct time-frequency representation of source level data. It should work for all possible arguments of the respective functions (except n_jobs, which is ignored for morlet/multitaper, if a list is passed).

Additional information

Note that the PR #6543 will be further maintained within this pull, as the SourceTFR class and changes in time_frequency/tfr often depend on each other.

There are also some things that still should be implemented:

  • economic computation, if stcs with a (kernel, sens_data) data field is passed
  • economic computation of lists for tfr_stockwell
  • frequency plotting in source space.

DiGyt added 30 commits June 6, 2019 14:36
Added Basic SourceTFR structure and Text

Signed-off-by: Dirk Gütlin <[email protected]>
Added Basic SourceTFR structure and Text

Signed-off-by: Dirk Gütlin <[email protected]>
Added basic tests to load VectorSourceEstimate into tfr_multitaper

Signed-off-by: Dirk Gütlin <[email protected]>
added tests for sourceTFR

Signed-off-by: Dirk Gütlin <[email protected]>
First steps to read SourceEstimates into multitapers

Signed-off-by: Dirk Gütlin <[email protected]>
tfr_multitaper now takes VectorSourceEstimates without error
tests were introduced to check whether it works

Signed-off-by: Dirk Gütlin <[email protected]>
updated computing tfr funcs with Source TFR
updated tests for this

Signed-off-by: Dirk Gütlin <[email protected]>
- updated source tfr
- added tests

Signed-off-by: Dirk Gütlin <[email protected]>
Signed-off-by: Dirk Gütlin <[email protected]>

# Conflicts:
#	mne/time_frequency/multitaper.py
added tests four SourceTFR

Signed-off-by: Dirk Gütlin <[email protected]>
Signed-off-by: Dirk Gütlin <[email protected]>
- Added tests
- Removed redundant calls in source_tfr.py

Signed-off-by: Dirk Gütlin <[email protected]>
cleaned up tfr and multitaper

Signed-off-by: Dirk Gütlin <[email protected]>
..to master level

Signed-off-by: Dirk Gütlin <[email protected]>
- introduced coverage for different cases of parameters

Signed-off-by: Dirk Gütlin <[email protected]>
- made functions accessible to SourceEstimates
- introduced tests for checking
- made sure data is produced correctly

Signed-off-by: Dirk Gütlin <[email protected]>
Signed-off-by: Dirk Gütlin <[email protected]>
- lists/generators/single stcs
- average
- itc
- power/ complex

Signed-off-by: Dirk Gütlin <[email protected]>
Signed-off-by: Dirk Gütlin <[email protected]>
also reverted _get_data

Signed-off-by: Dirk Gütlin <[email protected]>
@DiGyt
Copy link
Contributor Author

DiGyt commented Aug 18, 2019

Is #6674 taking over this one?
Sorry #6673

@massich
No, I just opened #6673 and #6672 up as draft PRs, to be merged after this one. I think this pull should really be the priority, and the other stuff can be added later.

- made lists and single stcs work
- introduced checks
- added to test
... as this is now covered in test_stfr_equivalence
stockwell is computed on sensor data to save time, and the full stc data is computed afterwards
to make stuff more comprehensible
in order to pass build docsa
# Conflicts:
#	doc/changes/latest.inc
# Conflicts:
#	doc/changes/latest.inc
...from mne/ to mne/time_frequency
... to prevent a deep circular import reference.
@DiGyt
Copy link
Contributor Author

DiGyt commented Sep 3, 2019

@agramfort
I tried to migrate the source_tfr.py module from mne/ to mne/time_frequency as you mentioned in #6673 (comment) (I also think that this idea makes a lot of sense). However this is causing a very nasty circular import reference, if we include in in time_frequency.__init__ and I'm not sure how to handle it in the best manner.
I thought, as you also would like to factorize everything with _BaseSourceEstimate ( #6673 (comment) ), we might just add the SourceTFR class to source_estimate.py, in order to keep everything together. But obviously, it would also make sense to keep SourceTFR in mne/time_frequency.

However, @agramfort @massich
The SourceTFR class does currently allow the possibility of storing data in the (kernel, sensor_data) form. But for now, this option is not even used, as the full tfr source time course is computed inside the time_frequency functions (in order to cope with the nonlinear steps that need to be performed after np.dot(kernel, sensor_data) ). So at the moment, the question is if we want to maintain the unused (kernel, sensor_data) representation for SourceTFRs.
If we do, I think it makes sense to make SourceTFR inherit stuff from _BaseSourceEstimate, and add it to the mne/source_estimate.py file as mentioned above. If we don't want to maintain this functionality, there will be less stuff to inherit from _BaseSourceEstimate, and therefore it would eventually be better to keep it a separate class.

So 2 questions, where i'd like to hear your opinions:

  1. Should we maintain the functionality of SourceTFRs supporting (kernel, sensor_data) type data?
  2. Should we add SourceTFR to mne/source_estimate.py or try to put it in mne/time_frequency/source_tfr.py?

Base automatically changed from master to main January 23, 2021 18:26
@DiGyt DiGyt closed this Feb 22, 2021
@DiGyt
Copy link
Contributor Author

DiGyt commented Feb 22, 2021

Closed: See Issue #6290

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.

5 participants