Skip to content

Conversation

@dengemann
Copy link
Member

self-explanatory, I think

@dengemann
Copy link
Member Author

cc @deep-introspection

@dengemann
Copy link
Member Author

@deep-introspection we could actually also add support for your polhemus files here. please share

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately the wrong montage ....

@dengemann
Copy link
Member Author

cc @t3on @christianbrodbeck this might also be interesting for you. Feedback welcome.

@deep-introspection
Copy link
Contributor

@dengemann Seems cool! Shall I send you a sample file? How can I help?

@dengemann
Copy link
Member Author

@deep-introspection are you around? We could try to find some time to move on with things. Today is open night at La Paillasse. If you have data for which you know how things are supposed to look like we could try and play with the montages. Adding polhemus is also a good idea.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 18ba184 on dengemann:montage into f4c6245 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 18ba184 on dengemann:montage into f4c6245 on mne-tools:master.

@deep-introspection
Copy link
Contributor

@dengemann I am not available tonight... There is a brainy event Sunday late afternoon at La Generale (http://www.lagenerale.fr/?p=4674), want to meet before in the corner(~2pm)?

@agramfort
Copy link
Member

@t3on can you give @dengemann a hand on this?

@wmvanvliet
Copy link
Contributor

What more is needed to complete this PR? Can I help?

@dengemann
Copy link
Member Author

@wmvanvliet @t3on let me review and see where we are. I think this PR is actually almost ready.

@dengemann
Copy link
Member Author

I rebased this branch. If I remember correctly a rotation of the coordinates is missing for one of the montages. Thanks to our new interpolation + topomap plots this should be easy to figure out.

@wmvanvliet
Copy link
Contributor

A few things that needed doing before I could merge it in my own fork:

  • moving your edits to viz.py to viz/montages.py
  • match against full montage name instead of part of it to remove ambiguity between M1 and M10 montages
  • use os.path.splitext instead of relying on filename[:4] to deal with file extensions

You can find these changes at my montage branch, pull from there if you like them.

@dengemann
Copy link
Member Author

@wmvanvliet you can send me a PR into this branch, how about that? Visit my fork and open a PR, I'm happy to review and incorporate your changes.

@wmvanvliet
Copy link
Contributor

took me a while how to make a PR from a specific fork/branch to a specific fork/branch, but I got it working now.

Marijn.

On 1 okt. 2014, at 12:25, Denis A. Engemann notifications@github.com wrote:

@wmvanvliet you can send me a PR into this branch, how about that? Visit my fork and open a PR, I'm happy to review and incorporate your changes.


Reply to this email directly or view it on GitHub.

Marijn van Vliet
w.m.vanvliet@gmail.com

@larsoner
Copy link
Member

larsoner commented Oct 2, 2014

Files should go in mne/data/montages instead of creating a new root directory mne/montages

@larsoner
Copy link
Member

larsoner commented Oct 2, 2014

Also needs a rebase

@dengemann dengemann force-pushed the montage branch 2 times, most recently from 4bc1efc to 42621a4 Compare October 15, 2014 16:25
@wmvanvliet
Copy link
Contributor

the easycaps coordinates are wrong. They are rotated 90 degrees along the x-axis.

@dengemann
Copy link
Member Author

@wmvanvliet yo, it turns out the montage was correct all the time (before the last commits). It's just that the sample dataset uses M11 not M10 as I seem to have figured. We need to rethink the example. M11 coordinates are not available as far as I know. Using standard 10-5 system we could have a mapping if we knew the standard channel names and the order but we don't. I'm now taking a look at the other EEG dataset that we have.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling c8a7c65 on dengemann:montage into 9bc5e41 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling e7a5907 on dengemann:montage into 5997ec8 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling e7a5907 on dengemann:montage into 5997ec8 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling c72ecc6 on dengemann:montage into 5997ec8 on mne-tools:master.

@agramfort
Copy link
Member

I am done here (i think)

let's wait for travis to be happy and let's merge !

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 217e1f4 on dengemann:montage into 5997ec8 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 0d0879b on dengemann:montage into 5997ec8 on mne-tools:master.

dengemann added a commit that referenced this pull request Oct 31, 2014
@dengemann dengemann merged commit 7e5b90e into mne-tools:master Oct 31, 2014
@dengemann
Copy link
Member Author

@wmvanvliet @agramfort thanks for your participation. One more EEG feature merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants