[MRG] Update read_montage docstring by cbrnr · Pull Request #4400 · mne-tools/mne-python

cbrnr · 2017-07-19T09:25:06Z

I've updated the docstring of the mne.channels.read_montage function. Specifically, I have

added the total number of channels in each montage,
re-formatted to make PEP8 happy,
switched easycap-M1 and easycap-M10,
and added the missing montage, which I renamed because the previous name was too long (if anyone wants to add a more detailed description for this montage let me know).

codecov-io · 2017-07-19T10:37:59Z

Codecov Report

Merging #4400 into master will decrease coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4400      +/-   ##
==========================================
- Coverage   82.75%   82.47%   -0.28%     
==========================================
  Files         349      349              
  Lines       63970    64291     +321     
  Branches     9851     9924      +73     
==========================================
+ Hits        52938    53026      +88     
- Misses       8306     8534     +228     
- Partials     2726     2731       +5

mmagnuski · 2017-07-19T10:39:43Z

oh, good you are switching easycap montages - it was a bit annoying. :)

mmagnuski · 2017-07-19T10:42:19Z

mne/channels/montage.py

+    GSN-HydroCel-256      HydroCel Geodesic Sensor Net (259 channels)
+    GSN-HydroCel-257      HydroCel Geodesic Sensor Net with Cz (260 channels)
+
+    10-5_EGI129           465 channels


this would need more description

I have no idea what kind of montage this is - it wasn't documented before. Anyone?

mmagnuski · 2017-07-19T10:45:50Z

mne/channels/montage.py

+    biosemi64             BioSemi cap with 64 electrodes (67 channels)
+    biosemi128            BioSemi cap with 128 electrodes (131 channels)
+    biosemi160            BioSemi cap with 160 electrodes (163 channels)
+    biosemi256            BioSemi cap with 256 electrodes (259 channels)


these are a bit confusing - 256 electrodes but 259 channels. I know you write at the top that some contain fiducials, but maybe its better to write (256 channel positions, 3 fiducial points)?

There is no property that distinguishes EEG channels from fiducials, this can only be inferred from the channel names. I don't know if it is really straightforward to make this distinction. The total number of channels in a given montage is easy and unique.

I'm just saying that they are not all channels - some are channel positions and some fiducial points so saying 259 channels is misleading.
In case of biosemi fiducials are ['Nz', 'LPA', 'RPA'] and in the case of EGI they all start with Fid. At least in the case of biosemi and EGI I think it would be useful to wirte (256 channel positions, 3 fiducial points) so it is clear what the montage contents are.

OK, I'll try to delineate where possible. What do we do with the strange new montage? It consists of half labels, half numbers?

@dengemann maybe?

BTW - montages starting with standard also have 3 fiducial points ('Nz', 'LPA', 'RPA').
I don't know about the strange new montage - maybe it was undocummented for a reason? maybe @Eric89GXL or @agramfort knows?

The standard montages seem to have more fiducials: Nz, LPA, RPA, A1, A2, M1, M2. That's what I meant, it is difficult to distinguish between real channel labels and fiducials, so I'm not sure if it's worth the effort. IMO it's sufficient to state this generally before, as it is currently implemented.

agramfort · 2017-07-19T11:12:31Z

no objection to merge.

let's merge when @mmagnuski is happy :)

cbrnr · 2017-07-19T12:10:56Z

There are two problematic montages:

standard_postfixed seems to have duplicate channel positions with different labels.
Same for standard_primed.

cbrnr · 2017-07-19T12:25:41Z

@mmagnuski I now list channels and fiducials separately for each montage.

Remaining issues:

The new and previously undocumented montage - what do we do about it?
standard_postfixed and standard_primed have duplicate channels positions

larsoner · 2017-07-19T13:17:50Z

I have no idea about those montages. Can git blame / history browsing tell you anything about who added them?

cbrnr · 2017-07-19T13:43:54Z

@dengemann added it a while ago

larsoner · 2017-07-19T13:50:51Z

Actually it's even older than that (need to look at the file pre-move to see its origin)

@dengemann any ideas about this standard_postfix.elc and standard_primed.elc why they have duplicate positions?

cbrnr · 2017-07-19T13:54:22Z

Where's that 10-5-System_Mastoids_EGI129.csd file in this commit?

larsoner · 2017-07-19T13:56:51Z

That commit I posted is for the weird standard_ ones (I thought that was mainly the concern), but the 10-5 is here

cbrnr · 2017-07-19T13:57:50Z

I need to learn how to browse the history of moved files on GitHub...

larsoner · 2017-07-19T14:04:47Z

There is probably a better way, but from your link I clicked on the commit hash (to see the changes), then clicked the "1 parent f2523e1" (toward the top), then "Browse files" (upper right), and navigated to mne/montages/ and saw the file there, and could browse its history.

cbrnr · 2017-07-19T14:07:26Z

Oh wow, didn't see the "parent" link - also you have to know where the file was previously... Thanks for pointing this out! 👍

larsoner · 2017-07-19T14:11:36Z

@cbrnr can you open a separate issue for the weird montages? This can be merged separately from dealing with that, right?

cbrnr · 2017-07-19T14:14:06Z

OK sure! Yes, these montages are separate issues.

larsoner · 2017-07-19T14:15:51Z

@mmagnuski are you happy, then? If so feel free to merge once the CIs are happy

agramfort · 2017-07-19T14:37:54Z

merge if you're happy

mmagnuski · 2017-07-19T17:21:30Z

I'm almost happy :)
The last thing is that A1, A2 (and probably M1 and M2) are channel positions not fiducials. See here for example.
@jona-sassenhagen correct me if I'm wrong.

So it seems that if fiducials are present in any montage there is always 3 of them.

@Eric89GXL - I dont have merge priviliges :(

jona-sassenhagen · 2017-07-19T18:27:54Z

Yes, M1 and M2 are mastoids. Together with A1 and A2, they're not fids, but electrodes.

cbrnr · 2017-07-20T07:30:11Z

OK done - could someone please restart the failed Travis job?

jona-sassenhagen · 2017-07-20T07:33:47Z

Restarted it

cbrnr · 2017-07-20T12:23:06Z

All CIs are happy, I don't know why coverage decreased, this should be unrelated. I guess ready to merge.

mmagnuski · 2017-07-20T14:24:50Z

Yes, I don't get codecov these days...
Anyway, everything seems to be ok, so I'm initiating merge 🚀
Thanks @cbrnr!

* Update read_montage docstring * List channels and fiducials separately for each montage * Fix number of fiducials

Update read_montage docstring

8b0ad8c

mmagnuski reviewed Jul 19, 2017

View reviewed changes

List channels and fiducials separately for each montage

bca5295

larsoner approved these changes Jul 19, 2017

View reviewed changes

cbrnr mentioned this pull request Jul 19, 2017

Defect montages shipped with MNE? #4403

Closed

Fix number of fiducials

5631a67

jona-sassenhagen closed this Jul 20, 2017

jona-sassenhagen reopened this Jul 20, 2017

cbrnr changed the title ~~Update read_montage docstring~~ [MRG] Update read_montage docstring Jul 20, 2017

mmagnuski merged commit c0013f3 into mne-tools:master Jul 20, 2017

cbrnr deleted the update_read_montage_docstring branch July 20, 2017 14:26

larsoner pushed a commit to larsoner/mne-python that referenced this pull request Aug 3, 2017

[MRG] Update read_montage docstring (mne-tools#4400)

ab93a3e

* Update read_montage docstring * List channels and fiducials separately for each montage * Fix number of fiducials

cbrnr mentioned this pull request Sep 21, 2017

Remove bad montage #4590

Merged

Uh oh!

Conversation

cbrnr commented Jul 19, 2017

Uh oh!

codecov-io commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mmagnuski commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

larsoner commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

larsoner commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

larsoner commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

larsoner commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

larsoner commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 19, 2017

Uh oh!

larsoner commented Jul 19, 2017

Uh oh!

agramfort commented Jul 19, 2017 via email

Uh oh!

mmagnuski commented Jul 19, 2017

Uh oh!

jona-sassenhagen commented Jul 19, 2017

Uh oh!

cbrnr commented Jul 20, 2017

Uh oh!

jona-sassenhagen commented Jul 20, 2017

Uh oh!

cbrnr commented Jul 20, 2017

Uh oh!

mmagnuski commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jul 19, 2017 •

edited

Loading