-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Combine gradiometer channels #11082
base: main
Are you sure you want to change the base?
Combine gradiometer channels #11082
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary comments/ideas!
mne/channels/layout.py
Outdated
def _combine_meg_grads(inst, method='rms'): | ||
"""Combine pairs of gradiometer channels.""" | ||
# TODO: | ||
# - modify coil_type and kind to a combined gradiometer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind should still be MEG I think. For coil_type I think we'd maybe want to add a 3016 here:
https://github.com/mne-tools/mne-python/blob/main/mne/io/constants.py#L929
We'll need to make a PR to fif-constants for this. We can do this later, but to make mne/io/tests/test_constants.py
happy you can for fif-constants, make a new branch with a commit that adds it, push to your fork, and tell test_constants to use your commit like here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I think this is done in 5fa25d1 and 1e87305 referencing this fork of fiff-constants https://github.com/AJQuinn/fiff-constants/tree/grad_combined
Not sure how critical the naming choice is?
"""Combine pairs of gradiometer channels.""" | ||
# TODO: | ||
# - modify coil_type and kind to a combined gradiometer | ||
# - modify units depending on method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the units should change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense for mean and rms - I'll ignore this for now.
# TODO: | ||
# - modify coil_type and kind to a combined gradiometer | ||
# - modify units depending on method | ||
# - need to handle noise_cov? projections? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- both should raise an error as you shouldn't need these. Same if you try to make_forward_solution
... but we can tackle these separately because there will be a lot of these "don't do this" paths
# - modify coil_type and kind to a combined gradiometer | ||
# - modify units depending on method | ||
# - need to handle noise_cov? projections? | ||
# - currently assumes inst only contains 'grads' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To start you could do inst.copy().pick(...)
, but in theory this should already live in as_type
in some reasonable way
# - modify units depending on method | ||
# - need to handle noise_cov? projections? | ||
# - currently assumes inst only contains 'grads' | ||
# - outsource return instance creation to specific methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also already live in as_type
. It would be good to reuse the code, deduplicating / making shared private functions as necessary
Ok - have some more updates which can be explored with this code.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a quick suggestion
picks = np.array(picks).reshape(-1, 2) # now [npairs x 2] | ||
new_chs = list() | ||
new_bads = list() | ||
for idx in range(picks.shape[0]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do:
for idx in range(picks.shape[0]): | |
for pair in picks: |
then do below
ch1 = inst.info['chs'][picks[idx, 0]] | ||
ch2 = inst.info['chs'][picks[idx, 1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ch1 = inst.info['chs'][picks[idx, 0]] | |
ch2 = inst.info['chs'][picks[idx, 1]] | |
ch1 = inst.info['chs'][pair[0]] | |
ch2 = inst.info['chs'][pair[1]] |
something is off about the plot. Since RMS makes the signal all positive, it should be plotting with a one-sided colormap. In our plotting code this will be handled by a variable called |
This is a work-in-progress start on general functions to combine meg gradiometer channels.
Idea is to allow users to combine gradiometer channels in the standard data instances, and eventually to replace the channel combining code used in mne/viz with this more general solution.
Would be great to get some feedback etc - any/all opinions welcome! @drammock @larsoner @agramfort
Some points for next steps...
_combine_meg_grads
assumes inst only contains 'grads'_combine_meg_grads
to specific class methodsReference issue
Relevant: #1280
What does this implement/fix?
I've implemented a private function in
mne.channels.layout
and linked it up toepochs.as_type
as a first use-case