MRG+1, ENH: Allow creating DigMontage for standard montages by massich · Pull Request #6714 · mne-tools/mne-python

massich · 2019-08-27T16:40:35Z

This works on #6461, It finds a way to create DigMontages from standard montage kind.

def read_standard_montage(kind: str) -> DigMontage

This could either be a new read_standard_montage or reuse read_montage.

lgtm-com · 2019-08-27T17:35:13Z

This pull request introduces 2 alerts when merging f9e1c2a into 3e60a9a - view on LGTM.com

new alerts:

2 for Unused import

codecov · 2019-08-28T09:18:11Z

Codecov Report

Merging #6714 into master will decrease coverage by 0.11%.
The diff coverage is 91.85%.

@@            Coverage Diff             @@
##           master    #6714      +/-   ##
==========================================
- Coverage   89.59%   89.47%   -0.12%     
==========================================
  Files         419      421       +2     
  Lines       75892    75987      +95     
  Branches    12430    12444      +14     
==========================================
- Hits        67994    67992       -2     
- Misses       5102     5176      +74     
- Partials     2796     2819      +23

lgtm-com · 2019-08-28T10:14:17Z

This pull request introduces 2 alerts when merging 525ae9d into 751347d - view on LGTM.com

new alerts:

2 for Unused import

lgtm-com · 2019-08-28T13:14:15Z

This pull request introduces 2 alerts when merging dc938e6 into 751347d - view on LGTM.com

new alerts:

2 for Unused import

lgtm-com · 2019-09-09T12:27:40Z

This pull request introduces 5 alerts when merging 59c703c into 17fe113 - view on LGTM.com

new alerts:

3 for Unhashable object hashed
2 for Unused import

lgtm-com · 2019-09-09T17:30:52Z

This pull request introduces 5 alerts when merging 36b184d into 80471f1 - view on LGTM.com

new alerts:

3 for Unhashable object hashed
2 for Unused import

mne/channels/tests/test_standard_montage.py

lgtm-com · 2019-09-09T19:01:54Z

This pull request introduces 3 alerts when merging 20d1c6f into 80471f1 - view on LGTM.com

new alerts:

3 for Unused import

lgtm-com · 2019-09-10T09:52:10Z

This pull request introduces 3 alerts when merging 2a873ec into 80471f1 - view on LGTM.com

new alerts:

3 for Unused import

lgtm-com · 2019-09-10T13:53:15Z

This pull request introduces 3 alerts when merging 6d14613 into dbed4f6 - view on LGTM.com

new alerts:

3 for Unused import

lgtm-com · 2019-09-10T15:45:32Z

This pull request introduces 4 alerts when merging e858120 into dbed4f6 - view on LGTM.com

new alerts:

3 for Unused import
1 for Variable defined multiple times

massich · 2019-09-11T10:02:15Z

I think that make_standard_montage or load_standard_montage is a better name than read_standard_montage.

I like make_standard_montage (just for coorence with make_dig_montage, make_dig_ponints whatever... despite the fact that its actually a loader)

agramfort · 2019-09-11T10:09:07Z

+1 for make_standard_montage as it's not indeed a read function as it does not take a fname as input

…

lgtm-com · 2019-09-11T10:37:19Z

This pull request introduces 1 alert when merging 938fe3e into 13c8e69 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

massich · 2019-09-11T12:05:23Z

This is the rendered example in the current stage. Only montages without fiducials should be rendered properly. But this is not the case. standards and mghs do render properly which means that those points are already in 'head'. easycap is wierdly off, and HydroCell and biosemi are behaving as expected.

Fixing all this.

agramfort · 2019-09-11T12:18:09Z

cool go go @massich !

…

lgtm-com · 2019-09-11T13:58:41Z

This pull request introduces 1 alert when merging 0024040 into 13c8e69 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

massich · 2019-09-11T15:04:45Z

There is something really funky that I don't understand.

Even for something that is montage.plot() works i.e 'mgh60' the points had not been making sense to me. In the following (from 1e41d1c) I'm plotting several things. The points read from the montage (in unk coord), in blue the transform version of the montage, and in white some points in 0.085 sphere 'cos the sphere uses scale rather than radious and is not calibrated in the plot. (but is useful to see where 0 origin is) in green the transformation.

lgtm-com · 2019-09-11T15:47:50Z

This pull request introduces 1 alert when merging 1e41d1c into 13c8e69 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

lgtm-com · 2019-09-12T10:15:09Z

This pull request introduces 4 alerts when merging bbb7c08 into e16d112 - view on LGTM.com

new alerts:

3 for Unhashable object hashed
1 for Module-level cyclic import

lgtm-com · 2019-09-12T12:55:08Z

This pull request fixes 3 alerts when merging 3cd7ca0 into 8cc11b5 - view on LGTM.com

fixed alerts:

3 for Unhashable object hashed

massich · 2019-09-12T13:42:47Z

let's see if I can clear my thoughts,

Despite the fact that plot_montage.py renders properly [missing-link], The channel locations are not proper because transforming something that is already in head, leads to points that are no longer in the sphere. Check the following (as before original montage is in red, and transformed montage in blue):

for kind in [
    'GSN-HydroCel-128',
    'biosemi128',
    'mgh60',
    'standard_1005',
]:

    montage = make_standard_montage(kind)
    trf_montage = transform_to_head(montage)

    _plot_dig_transformation(trf_montage, montage, title=kind)

lgtm-com · 2019-09-12T14:08:17Z

This pull request introduces 1 alert and fixes 3 when merging 0252dd3 into 8cc11b5 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

fixed alerts:

3 for Unhashable object hashed

massich · 2019-09-12T14:19:06Z

Another way of seeing it (head on the left, unk on the right):

lgtm-com · 2019-09-12T15:08:13Z

This pull request introduces 1 alert and fixes 3 when merging 31405e8 into 8cc11b5 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

fixed alerts:

3 for Unhashable object hashed

lgtm-com · 2019-09-12T15:39:42Z

This pull request introduces 1 alert when merging c71ebf3 into 8cc11b5 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

lgtm-com · 2019-09-12T18:43:19Z

This pull request introduces 1 alert when merging 52ad108 into 812f8c4 - view on LGTM.com

new alerts:

1 for Module-level cyclic import

agramfort · 2019-09-13T08:59:36Z

mne/channels/tests/test_montage.py

                         if ch['ch_name'].startswith('EEG')])
-    raw.set_montage('mgh60')  # test loading with string argument
+    with pytest.deprecated_call():
+        raw.set_montage('mgh60')  # test loading with string argument


this should still work but via the DigMontage route

massich · 2019-09-13T09:22:24Z

Here is the rendered version of the example. Maybe we want to add couple words of why the points look a bit off some times. But this can be done later.

https://15297-1301584-gh.circle-artifacts.com/0/dev/auto_examples/visualization/plot_montage.html#sphx-glr-auto-examples-visualization-plot-montage-py

massich · 2019-09-13T11:27:09Z

@agramfort suggested IRL to merge this one. There #6764 #6765 should follow this one, and maybe allow to register the standard montages.

…s#6714) Adds `make_standard_montage(kind: str) -> DigMontage`; note: `'kind'` has to be valid builtin standard montage and `DigMontage` is in `'head'`

massich force-pushed the deprecate_montage branch from 525ae9d to dc938e6 Compare August 28, 2019 12:18

massich force-pushed the deprecate_montage branch from dc938e6 to 59c703c Compare September 9, 2019 11:51

massich commented Sep 9, 2019

View reviewed changes

mne/channels/tests/test_standard_montage.py Outdated Show resolved Hide resolved

massich force-pushed the deprecate_montage branch from e7980ab to 2a873ec Compare September 10, 2019 08:57

massich mentioned this pull request Sep 12, 2019

[RFC] render API #6760

Closed

Joan Massich added 11 commits September 12, 2019 20:13

move import

4f6a706

wip

7271aa1

ups

fe948e9

try

7e65fbe

just render! the thing

8c10126

cleaner hydrocel

fa57bb8

make biosemi similar to easycap

1607bca

UPS: actually display the differences between 'head' and 'unk'

7a6d73d

ups

8154444

..

0aaf59f

simplify it !

52ad108

massich force-pushed the deprecate_montage branch from 7ee8c7a to 52ad108 Compare September 12, 2019 18:13

agramfort and others added 4 commits September 13, 2019 09:30

more

5945831

fix some XXX

fe2d683

update API ref

9c2e60b

wip

4a4a12f

agramfort marked this pull request as ready for review September 13, 2019 08:37

agramfort reviewed Sep 13, 2019

View reviewed changes

massich force-pushed the deprecate_montage branch from d0c3c8e to 4a4a12f Compare September 13, 2019 09:02

Joan Massich added 2 commits September 13, 2019 11:17

cleanup

33e2f27

update whats new

c600a5c

pep

db37706

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

[squash mne-tools#6714] to remove

e73c184

massich changed the title ~~ENH: Allow creating DigMontage for standard montages~~ MRG+1, ENH: Allow creating DigMontage for standard montages Sep 13, 2019

massich merged commit e99226e into mne-tools:master Sep 13, 2019

massich deleted the deprecate_montage branch September 13, 2019 13:52

Uh oh!

Conversation

massich commented Aug 27, 2019

Uh oh!

lgtm-com bot commented Aug 27, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

lgtm-com bot commented Aug 28, 2019

Uh oh!

lgtm-com bot commented Aug 28, 2019

Uh oh!

lgtm-com bot commented Sep 9, 2019

Uh oh!

lgtm-com bot commented Sep 9, 2019

Uh oh!

Uh oh!

lgtm-com bot commented Sep 9, 2019

Uh oh!

lgtm-com bot commented Sep 10, 2019

Uh oh!

lgtm-com bot commented Sep 10, 2019

Uh oh!

lgtm-com bot commented Sep 10, 2019

Uh oh!

massich commented Sep 11, 2019

Uh oh!

agramfort commented Sep 11, 2019 via email

Uh oh!

lgtm-com bot commented Sep 11, 2019

Uh oh!

massich commented Sep 11, 2019

Uh oh!

agramfort commented Sep 11, 2019 via email

Uh oh!

lgtm-com bot commented Sep 11, 2019

Uh oh!

massich commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Sep 11, 2019

Uh oh!

lgtm-com bot commented Sep 12, 2019

Uh oh!

lgtm-com bot commented Sep 12, 2019

Uh oh!

massich commented Sep 12, 2019

Uh oh!

lgtm-com bot commented Sep 12, 2019

Uh oh!

massich commented Sep 12, 2019

Uh oh!

lgtm-com bot commented Sep 12, 2019

Uh oh!

lgtm-com bot commented Sep 12, 2019

Uh oh!

lgtm-com bot commented Sep 12, 2019

Uh oh!

agramfort Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

massich Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

massich commented Sep 13, 2019

Uh oh!

massich commented Sep 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 28, 2019 •

edited

Loading

massich commented Sep 11, 2019 •

edited

Loading