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: Make get_head_mri_trans() respect datatype and suffix attributes of MEG BIDSPath #969

Merged

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Feb 25, 2022

get_head_mri_trans() currently blindly replaces the datatype and suffix of the provided MEG BIDSPath with 'meg'. However, when working with derivative data, the suffix (and maybe the datatype too? needs to be discussed!) might be different, causing the function to fail as it will create a BIDSPath that points to a non-existent file.

The change here makes it such that if the provided MEG BIDSPath already has a suffix and / or datatype attribute set, they will not be modified. Only if the attributes are not set, we will fall back to 'meg'.

This change is required to use get_head_mri_trans() with Maxwell-filtered data produced by the MNE-BIDS-Pipeline.

The problem was discovered yesterday while working with @SophieHerbst.

WIP:

  • Tests
  • Changelog entry
  • Decide whether we really need this for datatype too, or whether the change for suffix alone is sufficient
  • Why do we limit this to MEG only anyway? Should it, theoretically, not work with any recording that has digitization points?

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

get_head_mri_trans() currently blindly replaces the datatype and suffix
of the provided MEG BIDSPath with 'meg'. However, when working with
derivative data, the suffix might be different, causing the function to
fail as it will create a BIDSPath that points to a non-existent file.

The change here makes it such that if the provided MEG BIDSPath has
a suffix and / or datatype attribute set, they will not be modified.
Only if the attributes are not set, we will fall back to 'meg'.
@hoechenberger hoechenberger changed the title Make get_head_mri_trans() respect datatype and suffix attributes Make get_head_mri_trans() respect datatype and suffix attributes of MEG BIDSPath Feb 25, 2022
@hoechenberger hoechenberger added this to the 0.10 milestone Feb 25, 2022
@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #969 (700843d) into main (d298752) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #969      +/-   ##
==========================================
- Coverage   94.58%   94.57%   -0.01%     
==========================================
  Files          25       25              
  Lines        3584     3597      +13     
==========================================
+ Hits         3390     3402      +12     
- Misses        194      195       +1     
Impacted Files Coverage Δ
mne_bids/dig.py 90.20% <50.00%> (-0.42%) ⬇️
mne_bids/read.py 96.16% <100.00%> (+0.04%) ⬆️
mne_bids/utils.py 96.53% <100.00%> (+0.10%) ⬆️

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 d298752...700843d. Read the comment docs.

@agramfort
Copy link
Member

can you add a test to make the pb more explicit to me? 🙏

@hoechenberger
Copy link
Member Author

@agramfort Yes, this is all still very much WIP, just wanted to get this started so I don't forget about it

@hoechenberger hoechenberger mentioned this pull request Mar 2, 2022
@hoechenberger hoechenberger marked this pull request as ready for review March 2, 2022 17:53
@hoechenberger
Copy link
Member Author

@alexrockhill @agramfort Do you have any idea why in get_head_mri_trans(), we always (sometimes silently, sometimes explicitly) demand that the BIDSPath that basically leads to the DigPoints must be for an MEG recording? In theory, iEEG and EEG should work as well, no? At least as long as we add a check that the coordinates are in the head system?

@hoechenberger
Copy link
Member Author

@agramfort I've added a test that hopefully demonstrates why this change is needed, at least until we've fully sketched out derivatives in BIDS and the BIDS-Pipeline.

@hoechenberger hoechenberger changed the title Make get_head_mri_trans() respect datatype and suffix attributes of MEG BIDSPath MRG: Make get_head_mri_trans() respect datatype and suffix attributes of MEG BIDSPath Mar 2, 2022
@alexrockhill
Copy link
Contributor

@alexrockhill @agramfort Do you have any idea why in get_head_mri_trans(), we always (sometimes silently, sometimes explicitly) demand that the BIDSPath that basically leads to the DigPoints must be for an MEG recording? In theory, iEEG and EEG should work as well, no? At least as long as we add a check that the coordinates are in the head system?

Can you explain what you mean by this? I don't follow.

mne_bids/utils.py Outdated Show resolved Hide resolved
@hoechenberger
Copy link
Member Author

I will try to add a test that provokes a failure by using an unsupported coordinate system

@alexrockhill
Copy link
Contributor

alexrockhill commented Mar 3, 2022

Ah I get what you mean now. I think the larger issue is that we have BIDSPath with things like datatype and space and we don't check that that is compatible with what's in the raw object thoroughly enough.

I'm not sure why it matters so much about the landmarks, I thought those were just used in the case of EEG data stored in CapTrak. Where else are they used?

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.

Ok to merge when green

thx @hoechenberger

@hoechenberger
Copy link
Member Author

@alexrockhill never mind, I discussed this with @agramfort and I think this approach here fixes things for us. We're dealing with files that do not conform to the BIDS naming conventions (they are derivatives) and we needed a way to use get_head_mri_trans() from them. I think all is working now!

@hoechenberger hoechenberger merged commit 048bd0e into mne-tools:main Mar 3, 2022
@hoechenberger hoechenberger deleted the head-mri-trans-derivatives branch March 3, 2022 17:28
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.

3 participants