[BUG, MRG] Fix coordinate frames #982

alexrockhill · 2022-03-08T17:30:59Z

Fixes leftovers from #980.

The fiducials were not removed before setting the montage, causing an unwanted transformation, now they are removed and then put back which is good because we want them in the sidecar
There was inefficiency in mne_bids.utils._get_landmarks because the tests for the same coordinate frame are done for raw.get_montage so it's better not to duplicate these
Whew, the end of the tutorial that I finished is long winded, but I think if you are given a template, there is no way to know if it's in surface RAS, voxels or scanner RAS, it's not in the definition, so if we are supporting this, I think it's a requirement to walk users through it

I think this cleans up a lot of loose threads but there is a lot of added narrative so if @hoechenberger or @agramfort or both want to read through it and make sure it's all necessary that would be really helpful. Other than that, I am much more confident that everything works now.

cc @adam2392 also because you work with this a lot too

Merge checklist

Maintainer, please confirm the following before merging:

codecov · 2022-03-08T17:41:53Z

Codecov Report

Merging #982 (455d488) into main (5798e49) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #982   +/-   ##
=======================================
  Coverage   94.99%   95.00%           
=======================================
  Files          25       25           
  Lines        3678     3665   -13     
=======================================
- Hits         3494     3482   -12     
+ Misses        184      183    -1

Impacted Files	Coverage Δ
mne_bids/utils.py	`95.74% <ø> (-0.36%)`	⬇️
mne_bids/dig.py	`98.08% <100.00%> (+0.50%)`	⬆️
mne_bids/read.py	`95.93% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5798e49...455d488. Read the comment docs.

alexrockhill · 2022-03-08T17:54:30Z

Ok, everything should pass and that's all I wanted to change.

I just added an error for consistency where previously there would have been a cryptic empty dictionary error if there were not meg landmarks in the raw file since I was changing that part of the code already.

examples/convert_ieeg_to_bids.py

adam2392 · 2022-03-08T19:29:06Z

examples/convert_ieeg_to_bids.py

+# MNE-Python, if written with :func:`mne_bids.write_raw_bids`, must have
+# an :attr:`mne_bids.BIDSPath.space` specified, and will be read in with
+# the montage channel locations set to the coordinate frame 'unknown'.


Is this true? Or perhaps it can just be made a little bit more clear.

Maybe it would also help to provide a direct link to the specification for that matter?

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

If it's not in MNE_TO_BIDS_FRAMES and there is no BIDSPath.space then it won't be written. It could default to 'Other' that's a -1 from me, I don't think Other should be in BIDS.

You get

warn("Coordinate frame could not be inferred from the raw object " "and the BIDSPath.space was none, skipping the writing of " "channel positions")

It's linked at the top, we could add it there, but the whole narrative is trying to get them not to save data in a template coordinate frame if possible so handing a list of template coordinate frames that close to the beginning of the example seems counterproductive

examples/convert_ieeg_to_bids.py

adam2392 · 2022-03-08T19:33:39Z

mne_bids/dig.py

-        raise ValueError('All HPI, electrodes, and fiducials must be in the '
-                         'same coordinate frame. Found: "{}"'
-                         .format(coord_frame))
+    if raw.get_montage() is None or raw.get_montage().get_positions() is None:


Why would montage not be None, but positions are None?

I got an error in the tests that positions were none I thought, let me double check

Okay I might be wrong. Maybe it's a downstream consequence of mne

mne_bids/dig.py

adam2392

Minor changes, but otw lgtm

agramfort · 2022-03-08T20:51:37Z

tell me when I should look here again.

alexrockhill · 2022-03-08T20:56:58Z

Ok, I added @adam2392 's comments, if you could review @agramfort that would be really helpful

alexrockhill · 2022-03-08T21:07:08Z

The narrative and code is really lengthy at the end to describe all the coordinate frame transforms. I think in the future we could refactor mne_bids.read.get_head_mri_trans to work with iEEG data but that would require the template being in BIDS format with fiducials in the sidecar. Since we don't have fsaverage that way in the sample data, maybe this isn't worth doing.

What might be worth doing eventually is to download all the templates, find fiducials on their T1s and save them somewhere in mne_bids. That way, montage could be transformed to mri by mne_bids.dig._read_dig_bids and then they could just use mne.compute_native_head_t to get the trans. The only thing is that we'd have to do the same circumventing the transform to head by raw.set_montage so that the data could be returned in mri so that they could compute the trans. That or read_raw_bids could return a trans as well which actually might be a nice convenience for EEG and MEG too.

adam2392 · 2022-03-08T21:19:28Z

What might be worth doing eventually is to download all the templates, find fiducials on their T1s and save them somewhere in mne_bids.

Seems like a good GSoC issue :p. Would be a pretty valuable contribution.

alexrockhill · 2022-03-08T21:59:34Z

I guess it wouldn't be too hard but the only thing is that you'd have to infer whether the channels.tsv coordinates were in surface RAS, voxels or scanner RAS. You could also have a keyword to specific with auto as the default.

alexrockhill · 2022-03-08T21:59:56Z

@adam2392 look okay now?

alexrockhill · 2022-03-08T22:42:31Z

I opened an issue on bids-specification which is really the most upstream point where this should be solved, maybe we'll get a bit of help

alexrockhill · 2022-03-09T16:15:34Z

Good to go? It would be nice to get this in as main is actually a bit broken right now (in the case of a montage with non-head coordinates being passed)

examples/convert_ieeg_to_bids.py

hoechenberger

Anyone, feel free to merge once CI goes green!

alexrockhill · 2022-03-09T18:05:03Z

Ok, all green

hoechenberger · 2022-03-09T18:45:20Z

Thanks, @alexrockhill!

fixes

466034c

fix flake, add error with test

a58603f

alexrockhill added 2 commits March 8, 2022 10:52

add trans

efed7f6

missed a few things, add summary

13029a1

adam2392 reviewed Mar 8, 2022

View reviewed changes

examples/convert_ieeg_to_bids.py Outdated Show resolved Hide resolved

adam2392 reviewed Mar 8, 2022

View reviewed changes

examples/convert_ieeg_to_bids.py Outdated Show resolved Hide resolved

adam2392 reviewed Mar 8, 2022

View reviewed changes

mne_bids/dig.py Show resolved Hide resolved

adam2392 requested changes Mar 8, 2022

View reviewed changes

adam changes

077d93c

adam was right positions won't be None

c5deae6

alexrockhill mentioned this pull request Mar 8, 2022

[ENH, BUG] BIDS Standard Template Coordinate Frame Definition bids-standard/bids-specification#1025

Closed

alexrockhill mentioned this pull request Mar 9, 2022

[ENH, MRG] Add BIDS Standard Template Transformations #983

Merged

6 tasks

adam2392 reviewed Mar 9, 2022

View reviewed changes

examples/convert_ieeg_to_bids.py Outdated Show resolved Hide resolved

adam2392 approved these changes Mar 9, 2022

View reviewed changes

adam suggested change

455d488

hoechenberger approved these changes Mar 9, 2022

View reviewed changes

hoechenberger merged commit 78e8de8 into mne-tools:main Mar 9, 2022

alexrockhill deleted the fix branch March 9, 2022 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG, MRG] Fix coordinate frames #982

[BUG, MRG] Fix coordinate frames #982

alexrockhill commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

alexrockhill commented Mar 8, 2022

adam2392 Mar 8, 2022

alexrockhill Mar 8, 2022

alexrockhill Mar 8, 2022

alexrockhill Mar 8, 2022

adam2392 Mar 8, 2022

alexrockhill Mar 8, 2022

adam2392 Mar 8, 2022

adam2392 left a comment

agramfort commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

adam2392 commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 9, 2022

hoechenberger left a comment

alexrockhill commented Mar 9, 2022

hoechenberger commented Mar 9, 2022

[BUG, MRG] Fix coordinate frames #982

[BUG, MRG] Fix coordinate frames #982

Conversation

alexrockhill commented Mar 8, 2022 • edited Loading

Merge checklist

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

alexrockhill commented Mar 8, 2022

adam2392 Mar 8, 2022

Choose a reason for hiding this comment

alexrockhill Mar 8, 2022

Choose a reason for hiding this comment

alexrockhill Mar 8, 2022

Choose a reason for hiding this comment

alexrockhill Mar 8, 2022

Choose a reason for hiding this comment

adam2392 Mar 8, 2022

Choose a reason for hiding this comment

alexrockhill Mar 8, 2022

Choose a reason for hiding this comment

adam2392 Mar 8, 2022

Choose a reason for hiding this comment

adam2392 left a comment

Choose a reason for hiding this comment

agramfort commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

adam2392 commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 9, 2022

hoechenberger left a comment

Choose a reason for hiding this comment

alexrockhill commented Mar 9, 2022

hoechenberger commented Mar 9, 2022

alexrockhill commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading