[wip] [I will rewrite hist] work-on montage

[massich]

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

[codecov]

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]

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]

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]

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]

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]

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]

@massich the API proposal is in

#3893 (comment)

Which step are you on?

[larsoner]

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]

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]

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]

@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]

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]

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]

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

[larsoner]

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]

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]

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

[larsoner]

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]

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