hichamjanati / groupmne

Multi-subject MEG and EEG source localization with MNE
MIT License
9 stars 3 forks source link

Add several multi-subject inverse models via Mutar #17

Closed hichamjanati closed 4 years ago

hichamjanati commented 4 years ago

This PR generalizes the current GroupLasso solver to include all multi-task models of Mutar https://hichamjanati.github.io/mutar/.

agramfort commented 4 years ago

shall I review @hichamjanati ?

hichamjanati commented 4 years ago

Yes please :)

On Tue, Dec 24, 2019, 4:43 AM Alexandre Gramfort notifications@github.com wrote:

shall I review @hichamjanati https://github.com/hichamjanati ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hichamjanati/groupmne/pull/17?email_source=notifications&email_token=AECC63HEHCYNM7YHX7YYF63Q2H7T7A5CNFSM4J63BY6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHTE26A#issuecomment-568741240, or unsubscribe https://github.com/notifications/unsubscribe-auth/AECC63DEQFXEBGN6PHGZVYTQ2H7T7ANCNFSM4J63BY6A .

codecov[bot] commented 4 years ago

Codecov Report

Merging #17 into master will decrease coverage by 14.12%. The diff coverage is 89.38%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #17       +/-   ##
===========================================
- Coverage   90.97%   76.85%   -14.13%     
===========================================
  Files          10       10               
  Lines         532      700      +168     
  Branches       78      111       +33     
===========================================
+ Hits          484      538       +54     
- Misses         30      146      +116     
+ Partials       18       16        -2
Impacted Files Coverage Δ
groupmne/solvers.py 19.46% <100%> (-70.71%) :arrow_down:
groupmne/tests/test_inverse.py 100% <100%> (ø) :arrow_up:
groupmne/group_model.py 100% <100%> (+12.71%) :arrow_up:
groupmne/tests/test_group.py 100% <100%> (+1.31%) :arrow_up:
groupmne/tests/conftest.py 100% <100%> (ø) :arrow_up:
groupmne/utils.py 68.24% <75.43%> (-12.5%) :arrow_down:
groupmne/inverse.py 85.79% <85.18%> (-14.21%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a50495a...858cc6c. Read the comment docs.

hichamjanati commented 4 years ago

for MNE users this API will still be too low level. compute_group_inverse should take as input a list of forward objects and a list of evoked object.

The compute_inv_data step that returns np arrays can be be private indeed so that the user only works with MNE objects.

hichamjanati commented 4 years ago

To hide the transition to numpy arrays and also avoid recomputing the aligned and filtered leadfields + permutations I created an InverseOperator class. Wdyt about this api @agramfort ?

agramfort commented 4 years ago

can you work with fwds assuming the fwds have been modified by prepare_forwards so no further check is needed? basically prepare_fowards will make you have new fwds that are compatible out of the box.

hichamjanati commented 4 years ago

I'm not sure whether I can modify instances of fwds by removing some vertices and reordering them. The souvenir I have from last year's adventures (https://github.com/mne-tools/mne-python/issues/5349) is that it requires hacking all mne objects (SourceSpaces, Forward.. ) to have vertices in a -not sorted- order.

agramfort commented 4 years ago

you will have a problem when you save the forward to disk. Otherwise you can do anything you want with the forward instances especially since you control the inverse solvers.

hichamjanati commented 4 years ago

True I forgot MNE objects are just dictionaries. Ok so we can add the gains / permutations as new attributes to fwd instances. Maybe we can avoid overwriting stuff in fwds so as not to have problems if the user calls single subject mne solvers on a modified fwd ?

agramfort commented 4 years ago

sure that would be a good idea.

agramfort commented 4 years ago

Please make it a test

agramfort commented 4 years ago

ok. Shall we merge this one as is or do it in this PR?

hichamjanati commented 4 years ago

I vote for merging this one once it's green to keep PRs independent + we need to discuss the preprocessing in mne before doing anything

agramfort commented 4 years ago

ok green !