Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX : fix ch_names in topomaps #1550

Merged
merged 2 commits into from
Sep 9, 2014

Conversation

agramfort
Copy link
Member

cc @mainakjas for #1548

@agramfort
Copy link
Member Author

@mainakjas let me know if this works.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling f1a1191 on agramfort:fix_names_topomap into e273498 on mne-tools:master.

@mainakjas
Copy link
Contributor

Thanks @agramfort! hmm ... this seems to be fine for ch_type=mag but I still see missing channels on the periphery when ch_type=grad (e.g., MEG 0922).

@agramfort
Copy link
Member Author

yes there is a bug as the grad names are not combined when computing their
norms.
What should be the name of the combined grads?

@mainakjas
Copy link
Contributor

MEG 092x, MEG 161x etc for mne.viz.plot_evoked_topomap? I want to visualize some stats -- so combining norms may not be ideal for mne.viz.plot_topomap ...

@agramfort
Copy link
Member Author

MEG 092x, MEG 161x etc for mne.viz.plot_evoked_topomap?

fine with me.

I want to visualize some stats -- so combining norms may not be ideal for mne.viz.plot_topomap ...

what's your suggestion? what is it exactly you want to visualize?

@mainakjas
Copy link
Contributor

On Mon, Sep 8, 2014 at 5:54 PM, Alexandre Gramfort <[email protected]

wrote:

MEG 092x, MEG 161x etc for mne.viz.plot_evoked_topomap?

fine with me.

I want to visualize some stats -- so combining norms may not be ideal
for mne.viz.plot_topomap ...

what's your suggestion?

show_names = True should show all channels in mne.viz.plot_topomap() since
this allows arbitrary distribution of sensor positions. And show_names =
True should show MEG 092x, MEG 161x etc for the rest of the
plot_***_topomap() functions.

what is it exactly you want to visualize?

classification accuracies. I'd prefer to take just the mean (rather than
rms) or display both the grad channels. Currently, the easiest way to do
this appears to be EvokedArray -> plot_topomap.


Reply to this email directly or view it on GitHub
#1550 (comment).

@agramfort
Copy link
Member Author

what's your suggestion?

show_names = True should show all channels in mne.viz.plot_topomap() since
this allows arbitrary distribution of sensor positions. And show_names =
True should show MEG 092x, MEG 161x etc for the rest of the
plot_***_topomap() functions.

the thing is that we automatically combine grads in the viz function.

what is it exactly you want to visualize?

classification accuracies. I'd prefer to take just the mean (rather than
rms) or display both the grad channels. Currently, the easiest way to do
this appears to be EvokedArray -> plot_topomap.

to me the cleanest thing would be to add a method to containers
(Epochs, Evoked) to combine grads and create a new container. This
method would take a param such as mse, mean, angle etc... We'd need a
new sensor type for combined sensors that is understood by topomap
functions...

@mainakjas
Copy link
Contributor

On Mon, Sep 8, 2014 at 7:35 PM, Alexandre Gramfort <[email protected]

wrote:

what's your suggestion?

show_names = True should show all channels in mne.viz.plot_topomap()
since
this allows arbitrary distribution of sensor positions. And show_names =
True should show MEG 092x, MEG 161x etc for the rest of the
plot_***_topomap() functions.

the thing is that we automatically combine grads in the viz function.

what is it exactly you want to visualize?

classification accuracies. I'd prefer to take just the mean (rather than
rms) or display both the grad channels. Currently, the easiest way to do
this appears to be EvokedArray -> plot_topomap.

to me the cleanest thing would be to add a method to containers
(Epochs, Evoked) to combine grads and create a new container. This
method would take a param such as mse, mean, angle etc... We'd need a
new sensor type for combined sensors that is understood by topomap
functions...

sounds like a plan :) for now, I think I can live with mse.


Reply to this email directly or view it on GitHub
#1550 (comment).

@agramfort
Copy link
Member Author

sounds like a plan :) for now, I think I can live with mse.

looking forward the moment you need it :)

@mainakjas
Copy link
Contributor

On Tue, Sep 9, 2014 at 4:36 PM, Alexandre Gramfort <[email protected]

wrote:

sounds like a plan :) for now, I think I can live with mse.

looking forward the moment you need it :)

so should we merge this once the grad names are fixed? I'll make a note of
the combining grads method in a new issue.


Reply to this email directly or view it on GitHub
#1550 (comment).

@mainakjas
Copy link
Contributor

oops, issue already exists: #1280 :)

@agramfort
Copy link
Member Author

here we go. Done on my side.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 0730bbd on agramfort:fix_names_topomap into 3dbbb95 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 0730bbd on agramfort:fix_names_topomap into 3dbbb95 on mne-tools:master.

@mainakjas mainakjas merged commit ad5f4f5 into mne-tools:master Sep 9, 2014
@mainakjas
Copy link
Contributor

works perfectly! thanks @agramfort

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.

3 participants