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

alexrockhill · 2022-03-09T17:04:18Z

I went ahead and downloaded all the T1s, marked their fiducials in voxels in Freeview and then computed the transforms. I think I'll wait until #982 merges first and then add these with a mne_bids.get_template_trans which will dramatically simplify the end. It was kind of cool to see all the different templates, there are definitely some interesting ones, maybe that will be something to incorporate in the future.

Also, I have no idea where to store these but they are 264KB total so I don't think it's too big of a deal where they go. It would be nice to store the T1s as well but then we'd probably have to make sure with their licenses.

Merge checklist

Maintainer, please confirm the following before merging:

alexrockhill · 2022-03-09T17:09:11Z

Oh also the T1s are here: https://drive.google.com/drive/folders/1rN-Rb7D6ZTyVKOc2WZ-AM3f3dspIhZtQ?usp=sharing and the script is in the first commit but then I deleted it and committed again so it doesn't accidentally end up merged.

larsoner · 2022-03-09T17:23:48Z

You only transformed from one template to another using fiducials? This seems much less accurate than using flirt or ants to compute a proper full affine

adam2392 · 2022-03-09T17:29:41Z

Oh also the T1s are here: https://drive.google.com/drive/folders/1rN-Rb7D6ZTyVKOc2WZ-AM3f3dspIhZtQ?usp=sharing and the script is in the first commit but then I deleted it and committed again so it doesn't accidentally end up merged.

Unrelated to the PR I suppose:

If BIDS doesn't take it, I would still be inclined to store it somewhere easily accessible; maybe mne-bids-data? I can imagine a future where leveraging the template T1s, one can verify their individual coordinates "look right". Then they can do any series of transformation from template to template and plot the corresponding coordinates and verify that it still "looks right" without having to leave their codebase (i.e. T1s are accessible programmatically).

Or... are the T1s themselves too big?

alexrockhill · 2022-03-09T17:30:49Z

There are a lot of them and the largest is 180 MB. I think that could be in a data repository though.

alexrockhill · 2022-03-09T17:33:05Z

I think because you need to anonymize and upload an individual T1 per subject for subject-specific coordinates and you don't need any anatomical data for the templates the lower bar to entry will make people want to share in template coordinates whether we recommend against it in mne-bids or not. So IMO, probably better to get on board and support it while still recommending to share in ACPC.

alexrockhill · 2022-03-09T17:35:10Z

744MB total for the T1s as I chose them (there were a few options for one or two that were coordinate-frame-aligned and Talairach isn't a T1, it's an aseg and quite a few are brain masked so not full T1s) in nii.gz, they could be mgz too and that might save a bit of space.

alexrockhill · 2022-03-09T17:47:51Z

You only transformed from one template to another using fiducials? This seems much less accurate than using flirt or ants to compute a proper full affine

This doesn't transform between templates, it just defines a head coordinate frame so that you can use MNE functions that require a trans and will facilitate transforming between scanner RAS, voxels and surface RAS depending on which coordinate frame the template channel locations are stored in. Sorry I missed this comment somehow @larsoner!

EDIT: I would argue against facilitating transformation between templates as that is going to run into copy-of-a-copy issues and is definitely not best practice; the data should only be transformed from an individual brain to a template brain and if it's not the right template IMO, I don't think you should try and transform again.

alexrockhill · 2022-03-09T17:54:18Z

Any thoughts on where to put the transforms, is this okay?

larsoner · 2022-03-09T18:26:44Z

Okay as long as it's just to properly define a head coord system this seems fine!

alexrockhill · 2022-03-10T22:02:10Z

Ok, this drastically reduces the complexity of the ieeg tutorial, so I think it's really good. I tried a few versions (e.g. just providing the trans) but this is all so complicated when you start mixing coordinate frames and units that I think it's better to just abstract everything. I'm not sure how everyone else feels about auto but I think because Freesurfer coordinate frames have scanner RAS == surface RAS this isn't in big danger of a mixup, which I added to the text. I think the ideal thing is someone grabs a dataset off OpenNeuro and it just works (that's kind of the whole point of BIDS) so I think this is the best solution until bids-standard/bids-specification#1025 gets resolved and hopefully they just pick a coordinate frame, probably voxels, that the electrodes.tsv data has to be in for BIDS standard templates.

hoechenberger · 2022-03-10T22:37:35Z

@alexrockhill Is this something we should try to squeeze into 0.10 as well or..?

alexrockhill · 2022-03-10T22:39:17Z

@alexrockhill Is this something we should try to squeeze into 0.10 as well or..?

It would probably be nice, it's mostly just cleaning up and making things nicer. Last PR though before the release definitely

alexrockhill · 2022-03-10T22:40:33Z

I think it should all be green now, I tested it thoroughly on my side, I just didn't configure accessing the data directly in the repository

codecov · 2022-03-10T22:48:52Z

Codecov Report

Merging #983 (477b021) into main (78e8de8) will increase coverage by 0.04%.
The diff coverage is 98.36%.

❗ Current head 477b021 differs from pull request most recent head 84b9e89. Consider uploading reports for the commit 84b9e89 to get more accurate results

@@            Coverage Diff             @@
##             main     #983      +/-   ##
==========================================
+ Coverage   95.00%   95.04%   +0.04%     
==========================================
  Files          25       25              
  Lines        3665     3715      +50     
==========================================
+ Hits         3482     3531      +49     
- Misses        183      184       +1

Impacted Files	Coverage Δ
mne_bids/dig.py	`98.06% <98.24%> (-0.03%)`	⬇️
mne_bids/__init__.py	`100.00% <100.00%> (ø)`
mne_bids/config.py	`97.59% <100.00%> (ø)`
mne_bids/write.py	`96.75% <100.00%> (ø)`

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 78e8de8...84b9e89. Read the comment docs.

alexrockhill · 2022-03-10T22:49:47Z

Ok, looks good to go, here's the artifact: https://6099-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_ieeg_to_bids.html

alexrockhill · 2022-03-10T23:20:45Z

Ok just a few typos, ready for review if someone has a chance

adam2392 · 2022-03-10T23:46:11Z

Looks pretty good to me. Something I wished for when I was still a phd student Haha.

alexrockhill · 2022-03-10T23:50:01Z

Looks pretty good to me. Something I wished for when I was still a phd student Haha.

Yeah, it took a bit of doing but I think well worth it.

doc/api.rst

mne_bids/dig.py

hoechenberger · 2022-03-11T08:15:07Z

mne_bids/dig.py

+    raw : mne.io.Raw
+        The MNE-Python Raw object with a montage in one of the BIDS standard
+        template coordinate frames (modified in place).


Passing an Info or even simply a DigMontage should suffice to get the trans, right? I think we should support this… or do you think it doesn't make sense, because usually the user will have a Raw (or Epochs or …) that they want to "convert", and they're not looking for just a trans?

I tried it with the montage originally but it was really cumbersome because you ended up getting, setting and then re-getting the montage in the examples. I'll change it to info though.

I tried to use info but I forgot it doesn't have the ContainsMixin so you can't get_montage although you can set it. @larsoner or @agramfort, is there a reason info can't get_montage? I'd be happy to add it in a quick PR if not but I assume so.

Or maybe get_montage should moved from the ContainsMixin to the MontageMixin, I'm not sure if that would break everything though

Ok I opened a PR and since there is about to be a release, this will immediately work with the stable branch of MNE (after the release). Since it's a new function, it won't break for anyone but will be a great reminder to upgrade to 1.0.

alexrockhill · 2022-03-11T20:17:45Z

Ok, I realized even if we merge the mixins PR, then it's a pain to depend on 1.0 so soon so I just copied it over. Is there a way to set a reminder to do something at the next release? At 0.11, we should switch it to just info.get_montage.

Also this should pass all the tests and be good to merge.

larsoner · 2022-03-11T20:27:21Z

I would make a:

# Remove once we depend on MNE-Python 1.0+
def _get_montage(info):
    try:
        info.get_montage
    except AttributeError:
        return RawArray(np.zeros((info['nchan'], 1)), info).get_montage()
    else:
        return info.get_montage()

adam2392

I'm not sure if the function template_to_head should be head_to_template. The Return doc string seems mismatched.

adam2392 · 2022-03-10T23:45:23Z

mne_bids/dig.py

+@verbose
+def template_to_head(raw, space, coord_frame='auto', unit='auto',
+                     verbose=None):
+    """Transform a BIDS standard template montage to the head coordinate frame.


Is it head to template or template to head?

The template montage is getting transformed to head and you need a head->mri transform to get it to work with functions that require a raw and a trans. So I think it's better the way it is. The trans is just secondary, the main thing it does is modify the info.

Ah yeah I see. Hard to do the mental gymnastics sometimes...

In that case, I wonder if it's worth adding a Notes section that explicitly warns the user there is literally no way of checking if you do something wrong (e.g. putting in the incorrect name in space parameter). Say someone puts fsaverage5 instead of fsaverage6.

Neither fsaverage5 or fsaverage6 are supported, those are deprecated haha.

You can plot the alignment with the T1 of the template space, if you put the wrong one, it will look off... We can add that later, there's an issue open because mne-bids doesn't support 3D plotting yet so I would have but not this PR

mne_bids/dig.py

adam2392 · 2022-03-11T20:41:23Z

Looks good to me. Can't do a thorough review of the code, but the details in the example are nice cuz I remember always having a million questions about these kinds of transformations.

alexrockhill · 2022-03-11T20:42:10Z

Ok, I think that's a bit uglier with all the template_to_head(...)[1] but if that was confusing you both @hoechenberger and @adam2392, then better to make it clear that the info is being modified in the return type. That's how functions like filter do it anyway so probably best.

alexrockhill · 2022-03-11T21:08:13Z

Ok to merge @adam2392?

agramfort

I also checked that the new files are also shipped with the source dist.

hoechenberger

Otherwise LGTM!

mne_bids/dig.py

hoechenberger · 2022-03-12T23:39:14Z

Doc build is failing, apparently due to an issue with MNE-NIRS?

/home/circleci/project/examples/convert_nirs_to_bids.py failed leaving traceback:
Traceback (most recent call last):
  File "/home/circleci/project/examples/convert_nirs_to_bids.py", line 31, in <module>
    import mne_nirs  # For convenient downloading of example data
  File "/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne_nirs/__init__.py", line 10, in <module>
    from . import statistics
  File "/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne_nirs/statistics/__init__.py", line 7, in <module>
    from ._glm_level_first import (RegressionResults, ContrastResults,
  File "/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne_nirs/statistics/_glm_level_first.py", line 19, in <module>
    from mne.channels.channels import ContainsMixin
ImportError: cannot import name 'ContainsMixin' from 'mne.channels.channels' (/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne/channels/channels.py)

cc @rob-luke

@alexrockhill I'm going ahead and merging this one, as the issue with the doc build seems unrelated to your changes. Thank you!

alexrockhill · 2022-03-12T23:42:10Z

Doc build is failing, apparently due to an issue with MNE-NIRS?

/home/circleci/project/examples/convert_nirs_to_bids.py failed leaving traceback:
Traceback (most recent call last):
  File "/home/circleci/project/examples/convert_nirs_to_bids.py", line 31, in <module>
    import mne_nirs  # For convenient downloading of example data
  File "/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne_nirs/__init__.py", line 10, in <module>
    from . import statistics
  File "/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne_nirs/statistics/__init__.py", line 7, in <module>
    from ._glm_level_first import (RegressionResults, ContrastResults,
  File "/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne_nirs/statistics/_glm_level_first.py", line 19, in <module>
    from mne.channels.channels import ContainsMixin
ImportError: cannot import name 'ContainsMixin' from 'mne.channels.channels' (/home/circleci/mne_bids_env/lib/python3.9/site-packages/mne/channels/channels.py)

cc @rob-luke

@alexrockhill I'm going ahead and merging this one, as the issue with the doc build seems unrelated to your changes. Thank you!

Should be a quick fix, I just moved that mixin to mne.io.meas_info.

hoechenberger · 2022-03-12T23:44:24Z

Should be a quick fix, I just moved that mixin to mne.io.meas_info.

Would be awesome if you could have a look at this if you have a few minutes to spare! Once the doc build is working again, I can release MNE-BIDS 0.10.

alexrockhill changed the title ~~[ENH] Add BIDS Standard Template Transformations~~ [ENH, WIP] Add BIDS Standard Template Transformations Mar 9, 2022

alexrockhill added 3 commits March 9, 2022 10:50

add template trans

4c6ee96

remove script [skip ci]

23c42ed

working version

0dac514

alexrockhill force-pushed the templates branch from 8aba535 to 0dac514 Compare March 10, 2022 21:57

alexrockhill changed the title ~~[ENH, WIP] Add BIDS Standard Template Transformations~~ [ENH, MRG] Add BIDS Standard Template Transformations Mar 10, 2022

forgot to remove my helper script

b18af9c

This was referenced Mar 10, 2022

Montage coordinate frames are saved wrongly #961

Open

add test for ieeg coord_frame read issue #961 #962

Closed

bug: write_raw_bids uses CapTrak coordsys for iEEG data (invalid) #723

Open

alexrockhill added 3 commits March 10, 2022 14:19

fix link

2b0874e

use importlib resources

3709a47

fix tests

287f279

alexrockhill added 2 commits March 10, 2022 14:55

add test

cdd4bfd

copy edit

040ed8d

hoechenberger reviewed Mar 11, 2022

View reviewed changes

fix path joining

c483681

alexrockhill mentioned this pull request Mar 11, 2022

[ENH, MRG] Refactor ContainsMixin and MontageMixin mne-tools/mne-python#10432

Merged

change to info

2742a84

adam2392 requested changes Mar 11, 2022

View reviewed changes

add info to returns to avoid confusion

477b021

adam2392 approved these changes Mar 11, 2022

View reviewed changes

agramfort approved these changes Mar 12, 2022

View reviewed changes

hoechenberger reviewed Mar 12, 2022

View reviewed changes

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

Richard suggestions

84b9e89

hoechenberger merged commit f45fe2e into mne-tools:main Mar 12, 2022

alexrockhill deleted the templates branch March 12, 2022 23:46

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

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

Uh oh!

Conversation

alexrockhill commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge checklist

Uh oh!

alexrockhill commented Mar 9, 2022

Uh oh!

larsoner commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam2392 commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexrockhill commented Mar 9, 2022

Uh oh!

alexrockhill commented Mar 9, 2022

Uh oh!

alexrockhill commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexrockhill commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexrockhill commented Mar 9, 2022

Uh oh!

larsoner commented Mar 9, 2022

Uh oh!

alexrockhill commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoechenberger commented Mar 10, 2022

Uh oh!

alexrockhill commented Mar 10, 2022

Uh oh!

alexrockhill commented Mar 10, 2022

Uh oh!

codecov bot commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexrockhill commented Mar 10, 2022

Uh oh!

alexrockhill commented Mar 10, 2022

Uh oh!

adam2392 commented Mar 10, 2022

Uh oh!

alexrockhill commented Mar 10, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrockhill commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Mar 11, 2022

Uh oh!

adam2392 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrockhill Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

alexrockhill commented Mar 9, 2022 •

edited

Loading

larsoner commented Mar 9, 2022 •

edited

Loading

adam2392 commented Mar 9, 2022 •

edited

Loading

alexrockhill commented Mar 9, 2022 •

edited

Loading

alexrockhill commented Mar 9, 2022 •

edited

Loading

alexrockhill commented Mar 10, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

alexrockhill commented Mar 11, 2022 •

edited

Loading

alexrockhill Mar 11, 2022 •

edited

Loading