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

[BUG, ENH, MRG] Pixels #976

Merged
merged 8 commits into from Mar 4, 2022
Merged

[BUG, ENH, MRG] Pixels #976

merged 8 commits into from Mar 4, 2022

Conversation

alexrockhill
Copy link
Contributor

@alexrockhill alexrockhill commented Mar 3, 2022

Implements part of the fix from here: #973

This both adds support for ieeg data in pixels and prevents montages from being identity mapped to head when it's not appropriate. Before pixels data was not read in whereas "Other" coordinate frame data was. I see no reason MNE users can't analyze data in pixels, they just have to handle the coordinate frame themselves and have it set as "unknown" like the rest of the unsupported coordinate frames. This accompanies the issue that when the coordinate frame was "unknown" in a BIDS dataset it was mapped to head with an identity transform. I think best just to leave these in "unknown" so that people aren't confused that it's in head for some reason when it really isn't.

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>"

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #976 (2bda182) into main (048bd0e) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 2bda182 differs from pull request most recent head 6d42709. Consider uploading reports for the commit 6d42709 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
+ Coverage   94.57%   94.63%   +0.06%     
==========================================
  Files          25       25              
  Lines        3597     3600       +3     
==========================================
+ Hits         3402     3407       +5     
+ Misses        195      193       -2     
Impacted Files Coverage Δ
mne_bids/config.py 97.46% <ø> (ø)
mne_bids/dig.py 91.37% <100.00%> (+1.16%) ⬆️

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 048bd0e...6d42709. Read the comment docs.

@alexrockhill
Copy link
Contributor Author

Ok, this should be good to go once everything comes back green, and allowing for pixels is 80% of the way there to allowing all the template coordinate frames since it sets the coordinate frame to unknown instead of head

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

doc/whats_new.rst Outdated Show resolved Hide resolved
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
doc/whats_new.rst Outdated Show resolved Hide resolved
doc/whats_new.rst Outdated Show resolved Hide resolved
mne_bids/tests/test_write.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger merged commit b26f062 into mne-tools:main Mar 4, 2022
@hoechenberger
Copy link
Member

Thanks @alexrockhill!

@alexrockhill alexrockhill deleted the pixels branch March 4, 2022 19:46
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.

None yet

4 participants