-
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
Colocated topomaps for OPM #13144
base: main
Are you sure you want to change the base?
Colocated topomaps for OPM #13144
Conversation
…per annoying alias bug
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
The best thing to do is probably to take a file from that dataset, crop it to be very short (or maybe better, just save a clean evoked response as it's already probably ~1MB!) and add it to mne-testing-data. We don't want to make tests depend on another dataset, but we can/should update that one.
Yikes, so the ones that are nominally radial aren't actually the most radial? Do the sensors get turned a lot? Seems like it would need to be off by 45 degrees at least, which is a lot. I didn't expect this to be the case in practice. We could in theory triage based on system type and channel name, but that doesn't seem all that satisfying. Does the resulting topomap look reasonable? Does it look better if you use your projection-onto-normal method (i.e., a weighted average of the colocated sensors in the direction of the ideal outward normal)? |
Cool, I created a PR to add an evoked OPM dataset to
Yes, the radial-identified sensors look pretty similar to just picking the sensors with the RAD label (see below). It's only 2 / 43 where RAD-labelled sensors weren't identified as the most radial. They are right next to each other on the side of the head, could have easily been a funny placement or helmet (@georgeoneill, @neurofractal). NBD. In our data, the max-radial picks the Z sensor 100% of the time, so I think it's a reasonable heuristic. What will be annoying is if different loc triplets should be used in different sensors types (eg fieldline). Guess it'll rely on people raising issues when it doesn't work. Down the line, maybe worth having somewhere in the It's actually pretty annoying to do the weighted average. Sensor radial scores are computed in a part of |
Good morning @harrisonritz, @larsoner! Just looking at the UCL dataset, it appears that we've got some false friends in the naming of the channels. The -RAD channels have a tangential orientation, whilst the -TAN have a radial profile. At this point in the development of the UCL system, we laid the sensors on their side for a lower-profile helmet, but the labelling of the sensors never changed to reflect that. Apologies for the confustion! |
No worries, anything to get them to work! When I used the x-axis orientation ( This does highlight the trickiness of inferring the right orientation axis from |
|
for more information, see https://pre-commit.ci
@georgeoneill I found that picking radial sensors using |
Hi @harrisonritz, my mistake, you are correct! I forgot what the order of the 12 elements of the 0-2: Position |
all good, thanks! just double-checking |
for more information, see https://pre-commit.ci
OK -- looks like it's passing major checks, sorry about the wait. |
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 some quick comments based on a preliminary look. Not 100% sure I followed the rename-then-drop mechanics, but had an idea that might make sense to simplify it and potentially make it more robust given that OPM names could contain .
and x
already. But if I'm off base on this then feel free to ignore (maybe after adding a test that uses rename_channels
to add what I thought could be problematic names and shows they are not problematic 😄 ). I won't be able to do a deeper review for another week or so but wanted to give some feedback/food for thought in the meantime...
@@ -1089,7 +1089,7 @@ def _pair_grad_sensors( | |||
return picks | |||
|
|||
|
|||
def _merge_ch_data(data, ch_type, names, method="rms"): | |||
def _merge_ch_data(data, ch_type, names, method="rms", modality="opm"): |
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.
We've been trying to add these places in our code nowadays
def _merge_ch_data(data, ch_type, names, method="rms", modality="opm"): | |
def _merge_ch_data(data, ch_type, names, method="rms", *, modality="opm"): |
for rem in sorted(to_remove, reverse=True): | ||
del merged_names[rem] | ||
data = np.delete(data, rem, 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.
Tiny bit more efficient because it will only allocate one new array instead of a bunch of them
for rem in sorted(to_remove, reverse=True): | |
del merged_names[rem] | |
data = np.delete(data, rem, 0) | |
for rem in sorted(to_remove, reverse=True): | |
del merged_names[rem] | |
data = np.delete(data, to_remove, axis=0) |
Channel names that have an x in them will be merged. The first channel in | ||
the name is replaced with the mean of all listed channels. The other | ||
channels are removed. |
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.
Is this description correct? I don't think it involves x
or taking a mean?
@@ -76,6 +76,7 @@ | |||
) | |||
|
|||
_fnirs_types = ("hbo", "hbr", "fnirs_cw_amplitude", "fnirs_od") | |||
_opm_coils = (8002,) |
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.
Don't use the int
but rather the corresponding FIFF.FIFFV_
instead. And there should be more entries, no?
# New names will have the form S1xS2 | ||
for set_ in overlapping_channels: | ||
idx = ch_names.index(set_[0]) | ||
new_name = ".".join(s for s in set_) |
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.
If we're going to join based on a .
and then check it later, better raise an error here if there is already a .
in the name. But maybe I'm missing a check somewhere else. And maybe a moot point if you take my idea above
# New names will have the form S1_D1xS2_D2 | ||
# More than two channels can overlap and be merged | ||
for set_ in overlapping_channels: | ||
idx = ch_names.index(set_[0][:-4]) | ||
new_name = "x".join(s[:-4] for s in set_) | ||
ch_names[idx] = new_name | ||
elif modality == "opm": | ||
# Modify the channel names to indicate they are to be merged | ||
# New names will have the form S1xS2 |
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.
Looks like it would be S1.S2
, no? And maybe the overlapping ones should just end with OPM-MERGE-REMOVE
or something, and the ones that we keep should just have the original channel names. Then to figure out which ones to remove you do a .endswith("OPM-MERGE-REMOVE")
(which seems way safer than checking for a x
or .
in the name, which could happen in real data) and keep the original names.
These look super helpful, thank you! I'll take a pass over the next week or so (going to a week-long conference on weds). I discovered draft mode, so hopefully now you won't get pinged for every pull. |
Reference issue (if any)
Adds features from #11579
What does this implement/fix?
This PR extends topomaps to accommodate OPM data with colocated sensors.
It modifies the code used for colocated fNIRS, which now handles both fnirs and OPM.
In contrast to fNIRS which averages the colocated sensors, for OPM sensors we just take the most radial sensor.
Additional information
mne.datasets.ucl_opm_auditory
. Test won't work as is (eg can't update UCL dataset, and wanted to confirm proper method), but the test is structured on the fnirs overlap test so probably won't need much moreloc
triplets for dual-axis vs triaxial sensors, based on comparingucl_opm_auditory
vs Princeton's triaxial opms. At UCL, they use*-RAD
and*-TAN
to indicate the two axes, and at Princeton we use* X
or* [X]
.*-RAD
channels are the most radial, but in some cases bothRAD
andTAN
have low radial scores, and TAN is the most radial sensorsAttaching my internal testing code to validate against picking
RAD
channels.Happy to share Princeton dataset if that's helpful.