Kernel param for apply inverse #6609

DiGyt · 2019-07-27T17:46:54Z

Reference issue

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

What does this implement/fix?

This change will allow the user to decide which data fields are returned from apply_inverse_epochs.
An according if statement was already implemented, but previously assigned to an impossible if case.
If full_data=True, the full data field will be returned (as usual). If full_data=False, the stc data will instead be returned as a tuple of (kernel, sensor_data). The real data is then automatically created when calling stc.data .

Additional information

Unfortunately, this only works for fixed orientations. For free orientations, this causes problems, since data is reshaped and multiplied with noise_nn in _make_stc, steps that else need to be added when calling stc.data .

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

…pply_inverse

- Added test to check the full_data param Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

codecov · 2019-07-27T18:19:51Z

Codecov Report

Merging #6609 into master will increase coverage by <.01%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##           master    #6609      +/-   ##
==========================================
+ Coverage   89.48%   89.48%   +<.01%     
==========================================
  Files         420      420              
  Lines       75601    75621      +20     
  Branches    12382    12385       +3     
==========================================
+ Hits        67654    67673      +19     
  Misses       5139     5139              
- Partials     2808     2809       +1

mne/minimum_norm/inverse.py

mne/minimum_norm/tests/test_inverse.py

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

DiGyt · 2019-07-29T20:42:19Z

it should be

if delayed

delayed means delay the application of the kernel

Oops, sure! Sorry, I forgot to switch it...

-switched True/False of delayed param Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

mne/minimum_norm/inverse.py

agramfort · 2019-07-30T08:01:18Z

cd mne git grep ":class:" yes I sure you can be more explicit in the docstring.

…

DiGyt · 2019-07-30T08:03:07Z

cd mne git grep ":class:" yes I sure you can be more explicit in the docstring.
…

OK, thanks a lot!

DiGyt · 2019-07-30T10:43:27Z

cd mne git grep ":class:" yes I sure you can be more explicit in the docstring.
…

OK, i didn't find any class that links directly to the SourceEstimate data ~~attributes~~ -> parameter. I was looking for something linking directly to the data explanation like :attr:`stc.data <mne.SourceEstimate>` . But if this is not possible, i'll just link the SourceEstimate.

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

…pply_inverse

mne/minimum_norm/inverse.py

agramfort

do you check / test somewhere that pick_ori must be normal if delayed=True?

mne/minimum_norm/tests/test_inverse.py

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

DiGyt · 2019-07-30T12:13:59Z

do you check / test somewhere that pick_ori must be normal if delayed=True?

Well technically source_ori="vector" and delayed=False does not lead to an error but is just omitted, so that in this case, the full data field is still created. Should i raise an error or just write in the documentation that it will be omitted (+ maybe a warning)?

...from get_epochs to epochs Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

mne/minimum_norm/inverse.py

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

- implemented computing of delayed for vector oris - changed docstring accordingly - enhanced tests to vector repr. Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

mne/minimum_norm/tests/test_inverse.py

…pply_inverse

merged/adapted to current master

massich · 2019-08-20T08:27:46Z

mne/source_estimate.py

+                     tstep=tstep, subject=subject)
+    else:
+        return Klass(data=data, vertices=vertices, tmin=tmin, tstep=tstep,
+                     subject=subject)


That's not how it works.

There should be a single return.

oh sry, I did it because of
#6609 (comment)

But now I see what you mean. I'll change that

f254a95 is not what I meant. I meant to keep a single
return Klass(.. to keep the logic we wrote in #6636. The entire purpose of #6636 was to keep this one simple.

Maybe I'm missing something. I'll give it a shot.

Actually I already wrote it for you here #6609 (comment)

But I think that e41236f is even cleaner.

Actually I already wrote it for you here #6609 (comment)

Yes, it just seemed to me that you'd be most in favor of:

+ stc = VectorSourceEstimate( + data=(data_rot, sens_data), vertices=vertices, tmin=tmin, + tstep=tstep, subject=subject + ) + else: + stc = VectorSourceEstimate( + data=data_rot, vertices=vertices, tmin=tmin, + tstep=tstep, subject=subject + )

But anyhow, I like your solution with the sentinel (tbh I didn't know about that practice). But it gets rid of that is_kernel bool and it's compact. So, for me, that's fine!

@massich

The code is meant to read by people (machines executing it is secondary), and usually, the flow is: look at the definition, look at the return check whatever else afterward. If you have 2 returns or you construct the return in 2 different places (which is the same thing) I need to understand what's in the middle of the function. Most of the time when you read a function it is not for debugging purposes I just want to skim through it and get a good grasp of whats going on.

If I have a single place that returns Klass and does something at data and this is in the return, I need no further reading. This function is a factory. Based on whatever, chooses which class to construct and modifies the data accordingly.

@DiGyt this is a nice read on sentinel https://treyhunner.com/2019/03/unique-and-sentinel-values-in-python/

massich

see https://github.com/mne-tools/mne-python/pull/6609/files#r315565439

DiGyt · 2019-08-20T11:50:42Z

see https://github.com/mne-tools/mne-python/pull/6609/files#r315565439

@massich thanks for the review. I adressed your problem.

massich · 2019-08-21T16:53:32Z

@DiGyt do you want to discuss e41236f?

(see https://github.com/mne-tools/mne-python/pull/6609/files/4ace21853dd3ffee714b147ce3829781a1be6b6a#diff-3623e826cb12cb2dfa80dddf3b746d02)

…pply_inverse

This is meant to simplify mne-tools#6609

larsoner · 2020-03-04T15:05:57Z

Moving the milestone for this to 0.21

DiGyt · 2021-02-22T15:00:15Z

Closed: See Issue #6290

DiGyt added 4 commits July 16, 2019 11:05

add full_data param to apply_inverse_epochs

1645de3

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Merge https://github.com/mne-tools/mne-python into kernel_param_for_a…

cba0585

…pply_inverse

Added test

5775bec

- Added test to check the full_data param Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Improved Tests, docstrings

3e35749

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

agramfort reviewed Jul 28, 2019

View reviewed changes

mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved

mne/minimum_norm/tests/test_inverse.py Outdated Show resolved Hide resolved

mne/minimum_norm/tests/test_inverse.py Outdated Show resolved Hide resolved

changed param name, added fixture

75cace0

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

corrected bool param semantics

3424e24

-switched True/False of delayed param Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

agramfort reviewed Jul 30, 2019

View reviewed changes

mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved

DiGyt added 2 commits July 30, 2019 13:08

improved docstring

a5349a3

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Merge https://github.com/mne-tools/mne-python into kernel_param_for_a…

246fbce

…pply_inverse

agramfort reviewed Jul 30, 2019

View reviewed changes

mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved

agramfort reviewed Jul 30, 2019

View reviewed changes

mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved

agramfort reviewed Jul 30, 2019

View reviewed changes

mne/minimum_norm/tests/test_inverse.py Outdated Show resolved Hide resolved

DiGyt and others added 2 commits July 30, 2019 13:47

Update mne/minimum_norm/inverse.py

15bfbda

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

Update mne/minimum_norm/inverse.py

5c6bec5

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

DiGyt added 2 commits July 30, 2019 14:17

changed fixture name...

a60e33c

...from get_epochs to epochs Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

fixed docstring

fd56d56

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

larsoner reviewed Jul 30, 2019

View reviewed changes

mne/minimum_norm/inverse.py Show resolved Hide resolved

larsoner reviewed Jul 30, 2019

View reviewed changes

mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved

DiGyt added 2 commits July 31, 2019 16:24

raise error if delayed and vector orientation

4eccbba

Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Delayed works for pick_ori = "vector" now

864c20d

- implemented computing of delayed for vector oris - changed docstring accordingly - enhanced tests to vector repr. Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

massich reviewed Aug 1, 2019

View reviewed changes

mne/minimum_norm/tests/test_inverse.py Show resolved Hide resolved

massich reviewed Aug 1, 2019

View reviewed changes

mne/minimum_norm/tests/test_inverse.py Outdated Show resolved Hide resolved

massich reviewed Aug 1, 2019

View reviewed changes

mne/minimum_norm/tests/test_inverse.py Outdated Show resolved Hide resolved

DiGyt added 3 commits August 8, 2019 19:45

rebase

17f9d05

Merge https://github.com/mne-tools/mne-python into kernel_param_for_a…

cc6e612

…pply_inverse

merged with master

4ace218

merged/adapted to current master

massich reviewed Aug 20, 2019

View reviewed changes

massich suggested changes Aug 20, 2019

View reviewed changes

DiGyt added 2 commits August 20, 2019 10:43

only one return statement in _make_stc

f254a95

fixed flake

6950b95

this should work

e41236f

massich self-requested a review August 21, 2019 16:59

massich approved these changes Aug 21, 2019

View reviewed changes

DiGyt added 2 commits August 22, 2019 09:45

change error message

6bd01a7

Merge https://github.com/mne-tools/mne-python into kernel_param_for_a…

782e61f

…pply_inverse

DiGyt mentioned this pull request Aug 22, 2019

[WIP] Examples for time frequency analysis on SourceEstimates #6673

Closed

Merge https://github.com/mne-tools/mne-python into kernel_param_for_a…

c3427b1

…pply_inverse

larsoner modified the milestones: 0.19, 0.20 Sep 11, 2019

alexrockhill pushed a commit to alexrockhill/mne-python that referenced this pull request Oct 1, 2019

MNT: refactor _make_stc (mne-tools#6636)

27be0a7

This is meant to simplify mne-tools#6609

larsoner modified the milestones: 0.20, 0.21 Mar 4, 2020

larsoner modified the milestones: 0.21, 0.22 Aug 31, 2020

larsoner modified the milestones: 0.22, 0.23 Dec 1, 2020

Base automatically changed from master to main January 23, 2021 18:26

DiGyt closed this Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel param for apply inverse #6609

Kernel param for apply inverse #6609

DiGyt commented Jul 27, 2019

codecov bot commented Jul 27, 2019 •

edited

DiGyt commented Jul 29, 2019

agramfort commented Jul 30, 2019 via email

DiGyt commented Jul 30, 2019

DiGyt commented Jul 30, 2019

agramfort left a comment

DiGyt commented Jul 30, 2019

massich Aug 20, 2019

DiGyt Aug 20, 2019

massich Aug 21, 2019

massich Aug 21, 2019

DiGyt Aug 21, 2019 •

edited

massich Aug 22, 2019

massich left a comment

DiGyt commented Aug 20, 2019

massich commented Aug 21, 2019 •

edited

larsoner commented Mar 4, 2020

DiGyt commented Feb 22, 2021

Kernel param for apply inverse #6609

Kernel param for apply inverse #6609

Conversation

DiGyt commented Jul 27, 2019

Reference issue

What does this implement/fix?

Additional information

codecov bot commented Jul 27, 2019 • edited

Codecov Report

DiGyt commented Jul 29, 2019

agramfort commented Jul 30, 2019 via email

DiGyt commented Jul 30, 2019

DiGyt commented Jul 30, 2019

agramfort left a comment

Choose a reason for hiding this comment

DiGyt commented Jul 30, 2019

massich Aug 20, 2019

Choose a reason for hiding this comment

DiGyt Aug 20, 2019

Choose a reason for hiding this comment

massich Aug 21, 2019

Choose a reason for hiding this comment

massich Aug 21, 2019

Choose a reason for hiding this comment

DiGyt Aug 21, 2019 • edited

Choose a reason for hiding this comment

massich Aug 22, 2019

Choose a reason for hiding this comment

massich left a comment

Choose a reason for hiding this comment

DiGyt commented Aug 20, 2019

massich commented Aug 21, 2019 • edited

larsoner commented Mar 4, 2020

DiGyt commented Feb 22, 2021

codecov bot commented Jul 27, 2019 •

edited

DiGyt Aug 21, 2019 •

edited

massich commented Aug 21, 2019 •

edited