[wip] [I will rewrite hist] work-on montage by massich · Pull Request #5836 · mne-tools/mne-python

massich · 2019-01-15T13:48:11Z

My attempt to get my head around the montage problems.
I will re-edit the PR, rewrite history and whatever..

codecov · 2019-01-16T10:53:02Z

Codecov Report

Merging #5836 into master will decrease coverage by 1.73%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #5836      +/-   ##
==========================================
- Coverage   89.02%   87.29%   -1.74%     
==========================================
  Files         407      409       +2     
  Lines       73778    73888     +110     
  Branches    12238    12244       +6     
==========================================
- Hits        65684    64501    -1183     
- Misses       5213     6537    +1324     
+ Partials     2881     2850      -31

massich

This commit should be reverted before merging. it points to OSF fork.

larsoner · 2019-02-14T19:20:06Z

This pull request introduces 4 alerts when merging a681cd2 into 12a8549 - view on LGTM.com

new alerts:

3 for Variable defined multiple times
1 for Unused import

Comment posted by LGTM.com

massich · 2019-02-15T16:34:24Z

I think EGI_256 is broken see https://11334-1301584-gh.circle-artifacts.com/0/dev/auto_examples/visualization/plot_montage.html (the .csd description was added in #1390)

larsoner · 2019-02-15T19:19:44Z

This pull request introduces 2 alerts when merging 80f926a into 44c2ba4 - view on LGTM.com

new alerts:

2 for Unused import

Comment posted by LGTM.com

larsoner · 2019-02-18T11:20:43Z

This pull request introduces 2 alerts when merging d74beb5 into 8b00fc9 - view on LGTM.com

new alerts:

2 for Unused import

Comment posted by LGTM.com

massich · 2019-02-18T18:52:38Z

I'm a bit lost with the montage problems. here is a rendering of my first approach: load all the Montages and see what happen.

The second approach (which is the current PR state) I created a dummy class contaning a valid info['dig'] and plot the alignment. Just to play with the user API.

@agramfort , @larsoner wdyt? what am I missing?

In my opinion, the problem resides in the fact that info['dig'] is linked to trans. If you think that this makes sense from the user point of view, we recode all montages and distribute the trans. And if someone needs something new, they would need to provide the registration.

larsoner · 2019-02-18T19:49:23Z

@massich the API proposal is in

#3893 (comment)

Which step are you on?

larsoner · 2019-02-18T19:58:10Z

This pull request introduces 5 alerts when merging 1bf9e88 into 8b00fc9 - view on LGTM.com

new alerts:

3 for Unused import
1 for Variable defined multiple times
1 for Unused exception object

Comment posted by LGTM.com

massich · 2019-02-19T09:13:12Z

I've created a dummy version of Digitization that contains a valid info['dig'] list. (Where each element has kind, ident, r, and coord_frame. More over all coord_frame are 4, which represent the head coordinate) However in order to create the plot I need a trans that maps head->mri. Or we should have an fsaverage such that he trans head->mri is the identity.

But here is where I get lost, why do we need an object whose internal representation matches 1-to-1 with info['dig']? what should dev_head_t be? which part of the info['dig'] do we need to populate in
info['chs'][idx]['loc'][:6] and why do we need to keep duplicated data + visualization flags to toggle between one and the other?

larsoner · 2019-02-19T10:21:09Z

This pull request introduces 5 alerts when merging 8570e86 into 4d932c2 - view on LGTM.com

new alerts:

3 for Unused import
1 for Variable defined multiple times
1 for Unused exception object

Comment posted by LGTM.com

agramfort · 2019-02-19T14:41:08Z

@larsoner do we always need dev_head_t ? is this just for MEG? Would we need it for EEG w or w/o a standard montage? thoughts anyone?

larsoner · 2019-02-20T16:35:14Z

Only need dev_head_t for MEG data.

why do we need an object whose internal representation matches 1-to-1 with info['dig']

Because we want all of the data from info['dig'] with some other convenience things. For example dig.plot() should just work, even if its internal representation of its data is this list of dict. Also, for example, dev_head_t computation comes into play for formats that give you points in two different coordinate frames, not just the head coord frame. In this case, matched points in the MEG coord frame and head coord frame can be used to give you the MEG<->Head transformation.

However in order to create the plot I need a trans that maps head->mri

We always have a trans parameter in our plotting functions to give the head<->MRI transformation, this should be the same. (It should not be part of the class.)

agramfort · 2019-02-20T16:48:59Z

but we agree that trans is not needed if you have dig points in MRI or head coordinate system and you work with only EEG. ok?

…

larsoner · 2019-02-20T16:56:32Z

Yes agreed, dev_head_t is a minor side note and in most cases should not be needed.

larsoner · 2019-02-21T12:19:04Z

This pull request introduces 7 alerts when merging 2c27331 into 7c21bd6 - view on LGTM.com

new alerts:

6 for Unused import
1 for Unused exception object

Comment posted by LGTM.com

larsoner · 2019-02-21T15:04:59Z

This pull request introduces 22 alerts when merging 0eb92d9 into 7c21bd6 - view on LGTM.com

new alerts:

19 for Unused import
2 for Unused local variable
1 for Unused exception object

Comment posted by LGTM.com

larsoner · 2019-02-22T12:19:50Z

This pull request introduces 15 alerts when merging 6631210 into 8dc4634 - view on LGTM.com

new alerts:

9 for Unused import
3 for Variable defined multiple times
2 for Unused local variable
1 for Unused exception object

Comment posted by LGTM.com

add test_digitation for convenience

larsoner · 2019-04-15T15:10:15Z

This pull request introduces 15 alerts when merging 8e7b839 into 4555011 - view on LGTM.com

new alerts:

9 for Unused import
3 for Variable defined multiple times
2 for Unused local variable
1 for Unused exception object

Comment posted by LGTM.com

larsoner · 2019-04-15T20:43:45Z

This pull request introduces 9 alerts when merging e27ab27 into ece7543 - view on LGTM.com

new alerts:

3 for Unused import
3 for Variable defined multiple times
2 for Unused local variable
1 for Unused exception object

Comment posted by LGTM.com

massich force-pushed the montage branch from 1be7daf to e59725c Compare January 16, 2019 14:40

massich force-pushed the montage branch from e59725c to ae203fb Compare February 14, 2019 17:48

massich commented Feb 14, 2019

View reviewed changes

massich force-pushed the montage branch 2 times, most recently from d422506 to 9433f7c Compare February 15, 2019 14:52

massich force-pushed the montage branch from 1bf9e88 to 8570e86 Compare February 19, 2019 08:55

massich mentioned this pull request Feb 21, 2019

[MRG+1] kit2fiff support for *.elp and *.hsp files #3320

Merged

larsoner added the Digitization label Mar 26, 2019

Joan Massich added 5 commits April 15, 2019 16:25

initial commit

39e2062

force update_ch_names

e4ba9d9

use fsaverage

d6bebae

use a modified version of MNE-sample-data

3f17e9b

iter

5625fa9

Joan Massich added 18 commits April 15, 2019 16:25

[skip travis][skip appveyor]

5dda6cd

[skip travis][skip appveyor] scaling factor

a865c09

[skip travis][skip appveyor]

1cd8822

[skip travis][skip appveyor] phonecall

4c94cb5

[skip travis][skip appveyor] more

e421dda

[skip travis][skip appveyor] NaN party

aac762f

[skip travis][skip appveyor] iter

fae495f

[skip travis][skip appveyor] Use Digitization

5386fe9

[skip travis][skip appveyor] add fsaverage transformation

f16c112

[skip travis][skip appveyor] wip

1ec24e9

[skip travis][skip appveyor] create Digitization from hsp, elp

483861d

[skip travis][skip appveyor] I forgot the codegit status

eb8658e

[skip travis][skip appveyor] wip

f48d520

[skip travis][skip appveyor] work with pos

fe38251

add test_digitation for convenience

wip

57a5ba9

works in the test but not in the tuto

32481bd

[skip ci] More notes on things I don't get

e93dcf8

[skip travis][skip appveyor] dependencies

8e7b839

massich force-pushed the montage branch from 6631210 to 8e7b839 Compare April 15, 2019 14:26

massich mentioned this pull request Apr 15, 2019

MRG : source reconstruction on EEG using fsaverage brain #6146

Merged

fix circle

e27ab27

massich pushed a commit to massich/mne-python that referenced this pull request Apr 15, 2019

use sample from mne-tools#5836

d52439d

massich closed this May 27, 2019

Uh oh!

Conversation

massich commented Jan 15, 2019

Uh oh!

codecov bot commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

massich left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Feb 14, 2019

Uh oh!

massich commented Feb 15, 2019

Uh oh!

larsoner commented Feb 15, 2019

Uh oh!

larsoner commented Feb 18, 2019

Uh oh!

massich commented Feb 18, 2019

Uh oh!

larsoner commented Feb 18, 2019

Uh oh!

larsoner commented Feb 18, 2019

Uh oh!

massich commented Feb 19, 2019

Uh oh!

larsoner commented Feb 19, 2019

Uh oh!

agramfort commented Feb 19, 2019 via email

Uh oh!

larsoner commented Feb 20, 2019

Uh oh!

agramfort commented Feb 20, 2019 via email

Uh oh!

larsoner commented Feb 20, 2019

Uh oh!

larsoner commented Feb 21, 2019

Uh oh!

larsoner commented Feb 21, 2019

Uh oh!

larsoner commented Feb 22, 2019

Uh oh!

larsoner commented Apr 15, 2019

Uh oh!

larsoner commented Apr 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 16, 2019 •

edited

Loading