ENH: Channel tags - Githubissues

larsoner commented 3 years ago

It would be nice to be able to tag channels, for example as belonging to a particular "group".

for MEGIN MEG, this could be 1) each triplet of sensors that share a common spatial location, and also 2) which selection they are a part of; e.g. MEG0111 could have tags ('011X', 'Left', 'temporal') or so (part of the 0111/2/3 triplet, in the Left-temporal group in .sel)
for iEEG, this would probably be sEEG shafts / ECoG grid
for OPMs, manufacturers creating one "sensor" with multiple sensitive volumes (even if spatially offset from one another) will probably want these groups

I propose we add to our new enhanced channel block (https://github.com/mne-tools/fiff-constants/pull/33) the ability to specify which group each channel belongs to. I think it makes sense to add a new entry like:

ch_tags     352 str -   "Channel tags"

@agramfort @alexrockhill @jasmainak thoughts? I think this will allow us to make visualization functions automagically work better, for example by:

channel groups each get their own color from the mpl color cycle in plot_alignment et al.
mne.channels.find_layout could construct linear layouts on a per-channel-group basis (e.g., for sEEG shafts) for use with mne.viz.plot_topo

If this makes sense, I'll open a parallel issue over in fiff-constants. But I figured we could think about and discuss it a bit here first!

welcome[bot] commented 3 years ago

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

jasmainak commented 3 years ago

Throwing some ideas: perhaps you might also want separate subplots for different channel groups in evoked.plot? Perhaps some harmonization with "channel selection" ?

larsoner commented 3 years ago

That could be an interesting separable viz enhancement that could build on / require this one

alexrockhill commented 3 years ago

My very tentative plan for the GUI for SEEG was to require channel names to be formatted like LAMY 7 and detect the electrode based on the shared pattern of string and warn that group-related functionality such as plotting 3D electrode shafts will be disabled without groups. Alternatively a dictionary of grouping could be passed.

That was just my first idea and I think this could be better but what would the API look like for defining the groups? Would they be detected automatically as for 3 MEG channels at the same location and the SEEG method describe above or would they have to be assigned manually? And would it be simpler just to make functions that do the automatic grouping in real time even though it's not as efficient since the alternative of assigning them manually seems onerous?

rob-luke commented 3 years ago

It would be nice to be able to encode channels as belonging to a particular "group".

+1 from me. For fNIRS this would be useful for encoding regions of interest, which are currently messy to handle. Or fibre bundles, which are often connected to different ADCs with different noise properties.

The current proposal makes sense. But would it be possible to have a single channel be part of multiple groups? For the ROI example, sometime channels fall within multiple ROIs.

agramfort commented 3 years ago

you would have an int that says to which channel group a channel belongs to and this would be optional?

yes I think it would be useful. But I think it should be a partition (you cannot belong to 2 groups) and the grouping should not be subjective. There should be one and only one way to group things.

larsoner commented 3 years ago

you would have an int that says to which channel group a channel belongs to and this would be optional?

I would prefer str to int as it's more general and descriptive.

I think it should be a partition

I'm not sure why we need to mandate this restriction, I'm not sure it helps with maintainability or usability (we can always have a partition=True default in set_channel_groups for example, but should support partition=False). I'd actually prefer to go the opposite direction and use a list of str (NAME_LIST) in case a channel actually should explicitly belong to more than one group. It's easy enough to add the restriction in a given I/O reader that what is created is always a partition if it should be one for that particular modality/format/sensor setup, but I don't see too much value in limiting the spec that all must be partitions just because we expect that some should be.

I could imagine a weird ECoG grid or sensor arrangement where a channel could belong to more than one group. Or for the purposes of viz, some "left temporal" channels could also be part of the "left parietal" group (i.e., the ones along the boundary) for example if you implemented layout definitions in the groups.

This is another reason list-of-str would be useful, for Neuromag you could encode both the triplet-level-group, and the selection-level-group. Given that there are multiple conceptual groupings even for Neuromag, it makes sense to me to allow multiple group definitions per channel.

There should be one and only one way to group things

Again I think this is great to implement at the I/O reader level, but I don't think we need to make this a limitation of the format spec. For example in principle there should be only one way to name channels (what the manufacturer set it to) and only one way to order them (what they were during acq) but in practice we allow quite some flexibility (rename_channels, reorder_channels, etc.) in configuring channels.

TL;DR I think the I/O readers that automatically set things can implement integers-as-strings if they want, and can make groups exclusive and singletons. But I don't think the spec should limit the definition this way, since it's easy enough to support much more (via list of str), it's easy enough to implement/check/enforce partitions when necessary, and I think there are use cases where we will want multiple groups per channel.

agramfort commented 3 years ago

ok for list of str (eg I would call it tags). On neuromag this could encode this information:

https://github.com/mne-tools/mne-python/blob/main/mne/data/mne_analyze.sel

so a channel can for example have the tag "left" and "temporal" and we should have convenient ways to select channels based on their tags.

thoughts?

drammock commented 3 years ago

ok for list of str (eg I would call it tags)

SGTM!

adam2392 commented 3 years ago

Chiming in here just cuz this feature would be great to add. It's also a metadata column in channels.tsv for iEEG-BIDS, so we could add it into downstream mne-bids.

https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/04-intracranial-electroencephalography.html#channels-description-_channelstsv

larsoner commented 3 years ago

Upstream issue opened: https://github.com/mne-tools/fiff-constants/issues/36

adam2392 commented 3 years ago

Jw would this feature allow also tagging the "anatomical location" of the IEEG electrodes too?

larsoner commented 3 years ago

I think tags could be user-configurable, so I don't see why not

mne-tools / mne-python

ENH: Channel tags #9591