Skip to content
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

[MRG] Allow electrodes.tsv to not fully match Raw object #704

Merged
merged 27 commits into from
Feb 28, 2021

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented Feb 10, 2021

PR Description

Closes: #701
Partially addresses: #605
Similar in style to #698

Summary

This PR allows electrodes.tsv contents to not fully match chs in Raw.

In addition, it fixes the repo to match recent clarifications to the spec and validator. Since the spec and validator was updated to constrain how space entity and *CoordinateSystem in the *coordsystem.json file must match, there are a few changes needed to make things work.

  1. Separation of "BIDS" coordinate frames and "MNE" coordinate frames: unfortunately, there just isn't a 1-1 matching, so we just do the best we can to match w/e coordinate frames possible. Note there is a loss going back and forth (e.g. captrak/electkneuromag all == 'head' in mne)
  2. Casing: BIDS coordinate system names are case-sensitive in spec and validator. We should therefore get rid of any "lower-casing", since strings must match exactly. A bit stringent... but it's in spec/validator.

iEEG coordinate frames:

  1. iEEG coordinate frames only support mni_tal for 'fsaverage'andunknownfor "Other"/, and rasforACPC. Note that ACPCin BIDS maps toras` in MNE, but not the other way around.
  2. fs_tal in mne-python is unused, per discourse discussion.

Unit tests

  • Some of the read/write unit tests dealing with space entity and coordinatesystem writing needed to be fixed.

Old Note (not implemented, so no need to read)

If we don't think having read_dig_bids as a public function is necessary, then I can take it out, but the general workflow in #605 related to iEEG electrode coordinate processing doesn't necessitate a Raw file. In addition, if mne-python coordinate system handling becomes more and more in-line with BIDS, then the DigMontage object will be valuable. For example, I'm imagining a future where:

# all workflows not requiring the Raw file itself yet
montage_mm = read_dig_bids()

# convert to voxel space
montage_vox = montage_mm.convert_to('voxel')

# maybe convert to tkras space
montage_mm = montage_mm.convert_to('tkras')

# maybe utilize the IntendedFor parameter to get the filepath to the Nifti image to interpret iEEG coords on
intended_img_fpath = montage_mm.get_intended_for()
t1w_img = nb.load(intended_img_fpath)

In summary, having an API to go from BIDS files -> DigMontage is a nice interface for people that then need to maybe do other stuff that doesn't need a Raw dataset.

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@adam2392
Copy link
Member Author

@hoechenberger I have taken out the explicit checks for subset of electrodes in raw, per #485

@hoechenberger
Copy link
Member

Thanks @adam2392, I will take a closer look at this tomorrow!

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Immiora can you please try if the changes in this PR solve your problem?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes overall seem fine to me, but I think that making a read_dig_bids function public we are catering to a very niche use case that can also be solved with two or three lines of code by the niche-user (adam :-P)

I am not 100% against it, but more public functions are not always better.

mne_bids/dig.py Show resolved Hide resolved
mne_bids/dig.py Show resolved Hide resolved
doc/whats_new.rst Outdated Show resolved Hide resolved
@adam2392 adam2392 changed the title [WIP] Allow electrodes.tsv to not fully match Raw object and implement read_dig_bids public function to get Montage [WIP] Allow electrodes.tsv to not fully match Raw object Feb 26, 2021
@adam2392
Copy link
Member Author

adam2392 commented Feb 26, 2021

The changes overall seem fine to me, but I think that making a read_dig_bids function public we are catering to a very niche use case that can also be solved with two or three lines of code by the niche-user (adam :-P)

I am not 100% against it, but more public functions are not always better.

Heh okay. Not 100% in on the idea, so I just removed it.

@adam2392 adam2392 changed the title [WIP] Allow electrodes.tsv to not fully match Raw object [MRG] Allow electrodes.tsv to not fully match Raw object Feb 26, 2021
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for removing the controversial part :)

doc/whats_new.rst Outdated Show resolved Hide resolved
mne_bids/dig.py Show resolved Hide resolved
mne_bids/dig.py Outdated Show resolved Hide resolved
mne_bids/dig.py Show resolved Hide resolved
mne_bids/dig.py Show resolved Hide resolved
mne_bids/dig.py Show resolved Hide resolved
Co-authored-by: Stefan Appelhoff <[email protected]>
doc/whats_new.rst Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 26, 2021

Codecov Report

Merging #704 (3635a0f) into master (74ac41a) will decrease coverage by 0.01%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
- Coverage   93.92%   93.91%   -0.02%     
==========================================
  Files          23       23              
  Lines        2749     2759      +10     
==========================================
+ Hits         2582     2591       +9     
- Misses        167      168       +1     
Impacted Files Coverage Δ
mne_bids/dig.py 90.57% <91.48%> (-0.93%) ⬇️
mne_bids/config.py 96.87% <100.00%> (ø)
mne_bids/read.py 97.87% <100.00%> (+0.04%) ⬆️
mne_bids/write.py 97.06% <100.00%> (+0.06%) ⬆️

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 45f3f39...3635a0f. Read the comment docs.

mne_bids/config.py Outdated Show resolved Hide resolved
mne_bids/config.py Outdated Show resolved Hide resolved
Co-authored-by: Richard Höchenberger <[email protected]>
@hoechenberger
Copy link
Member

@adam2392 Tests are failing. Aside from this, LGTM

@adam2392
Copy link
Member Author

@adam2392 Tests are failing. Aside from this, LGTM

Don't get mad at me, but the tests are failing because I think there's just some complications in earlier versions (due to prolly me), but with the latest constraints from the validator and spec, the "correct" way of doing things is at least more clear. However, had to adjust a few more lines... :p

Pushing up soon.

examples/convert_ieeg_to_bids.py Outdated Show resolved Hide resolved
mne_bids/config.py Show resolved Hide resolved
mne_bids/config.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
mne_bids/tests/test_write.py Outdated Show resolved Hide resolved
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from this one comment, LGTM

@hoechenberger
Copy link
Member

Thanks, @adam2392!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Size of electrodes vs size of channels
4 participants