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

Passing trans to mne.gui.coregistration() should automatically derive the MRI fiducials #10208

Closed
hoechenberger opened this issue Jan 15, 2022 · 23 comments

Comments

@hoechenberger
Copy link
Member

Currently, the following code will estimate the fiducials from fsaverage and ask me to (re)place them; however, with a trans provided, deriving the MRI fiducials is very easy and is actually what I'd expect to be done automatically. I shouldn't have to pick fiducials again if a trans has already been generated!

import os.path as op
import mne
from mne.io import read_info

data_path = mne.datasets.sample.data_path()
subjects_dir = op.join(data_path, 'subjects')
subject = 'sample'

fname_raw = op.join(data_path, 'MEG', subject, subject + '_audvis_raw.fif')
fname_trans = op.join(data_path, 'MEG', subject, 
                      f'{subject}_audvis_raw-trans.fif')

mne.gui.coregistration(
    subject='sample',
    subjects_dir=subjects_dir,
    inst=fname_raw,
    trans=fname_trans,
)

cc @GuillaumeFavelier

@larsoner
Copy link
Member

You mean that the digitized head space ones should be transformed to MRI coordinates? I disagree. I think for every MRI, someone should ideally manually click on the head where the fids are. There will be some error in every head digitization, and this should generally be higher than what you get with a manual click on the right location on the head surface.

@hoechenberger
Copy link
Member Author

hoechenberger commented Jan 17, 2022

I think there's some confusion here either on my or on your side 😅 Or my workflow is non-standard. I'll try to explain in the following what my issues are with the current implementation.

  1. Working with @SophieHerbst, we created a BIDS dataset containing participants' MEG data and anatomical MR scans.
  2. At this stage, the MR data doesn't contain any information on the anatomical landmarks. The MEG data contains digitization points.
  3. I then run FreeSurfer's recon-all to generate the surfaces from the anatomical scan.
  4. Now, I can start MNE's coregistration UI. It asks me to place the fiducial points on the MRI. After that, I can run the familiar fitting procedures to generate a transformation matrix to convert between mri ("surface RAS") and head.
  5. Upon closing of the coreg window, I retrieve this transformation matrix, transform the digitized fiducial coordinates (from the MEG file) to MRI voxel coordinates, and store them in the BIDS dataset in a sidecar file for the MR scan. My job is done: the BIDS data now contains fiducial point coordinates for both, the anatomical and the electrophysiological recordings.
  6. Now, @SophieHerbst wants to review what I did, and potentially try out if she can improve the fit / transformation matrix ever so slightly. So we recover the transformation matrix from the BIDS dataset via get_head_mri_trans() and pass it to mne.gui.coregistration() via the trans parameter.
  7. The GUI does load the trans; however, it still estimates initial MRI fiducial coordinates based on fsaverage and asks her to place them on the head, effectively rendering the imported transformation matrix incorrect unless she places the points exactly where I placed them (i.e., it's virtually impossible to do so)

Now I realized that CoregUI acceptes an mri_fiducials, which could be used to pass MR fiducial points. However:

  1. mne.gui.coregistration() doesn't seem to expose this parameter
  2. If one were able to pass both, mri_fiducials and trans, one could again end up with a mismatch (i.e., with an invalid transformation matrix); also in my case, I would have to transform the coordinates from head (MEG data digpoints) or voxels (BIDS metadata) to mri manually beforehand, which seems cumbersome and error-prone.

So currently there's no way for @SophieHerbst to review my work and "pick up where I left off" – well, alright, there's this functionality to save and load fiducials in the GUI, but this would complicate things much more than necessary IMHO.

I hope this made things a little clearer? Also happy to have a quick video call to discuss this!

@larsoner
Copy link
Member

larsoner commented Jan 17, 2022

  1. Now, I can start MNE's coregistration UI. It asks me to place the fiducial points on the MRI.

There should be a step 4.5 here where you click the "save" button. In other words, your manually-clicked-on-the-dense-head-surface MRI fiducial points should be the most accurate ones available, EDIT: and they should be saved to subject/bem/subject-fiducials.fif when you click "save". And importantly, they are independent of the digitization (and any associated error) that was done during the MEG data acquisition.

  1. Upon closing of the coreg window, I retrieve this transformation matrix, transform the digitized fiducial coordinates (from the MEG file) to MRI voxel coordinates, and store them in the BIDS dataset in a sidecar file for the MR scan.

Instead of saving the digitized-fiducial-points-transformed-to-MRI-space, you should save/use the above.

The GUI does load the trans; however, it still estimates initial MRI fiducial coordinates based on fsaverage and asks her to place them on the head

If you saved the BIDS dataset properly with the MRI-coordinate fiducials (ideally not transformed from dig, but manually placed, as I suggest above) then to me MNE-BIDS should write subject/bem/subject-fiducials.fif when loading the dataset or whatever. Then mne coreg -s subject -d ... will automatically see it's there, and not estimate from MNI or require the user to click.

@larsoner
Copy link
Member

larsoner commented Jan 17, 2022

... in other words, the "standard workflow" as I understand it is for any given MRI, you click once accurately EDIT: then clicke "save" to make the subject/bem/subject-fiducials.fif, and then any time you later mne coreg with this structural MRI it's detected and used. So either MNE-BIDS should write this file, or you should send this file to collaborators. But I don't see what we should change at the MNE end -- the subject-fiducials.fif is the MNE-Python "standard" way to fix the MRI fiducials.

@hoechenberger
Copy link
Member Author

hoechenberger commented Jan 17, 2022

in other words, the "standard workflow" as I understand it is for any given MRI, you click once accurately EDIT: then clicke "save" to make the subject/bem/subject-fiducials.fif, and then any time you later mne coreg with this structural MRI it's detected and used. So either MNE-BIDS should write this file, or you should send this file to collaborators.

Thanks @larsoner, I didn't know MNE would auto-retrieve the fiducials then.

Still, I'm not happy 🙈

In BIDS, I cannot store a transformation matrix. So what we have in BIDS metadata is:

  • digitized head points / fiducials for MEG data
  • fiducials for MR scan

Currently (i.e., current MNE-BIDS workflow!) these two sets of points implicitly determine the transformation matrix: we derive it from the two via get_head_mri_trans()

To store a transformation, I therefore have to

  • keep one set of fiducial coordinates fixed (either the digitized points in the MEG data, or the MRI fiducials)
  • apply the trans
  • store the respective transformed coordinates

Only then will I be able to later retrieve the transformation matrix again.

Does my problem become a little clearer?

cc @sappelhoff

@larsoner
Copy link
Member

In BIDS, I cannot store a transformation matrix. So what we have in BIDS metadata is:... fiducials for MR scan

This should be enough to create subject/bem/subject-fiducials.fif.

Currently (i.e., current MNE-BIDS workflow!) these two sets of points implicitly determine the transformation matrix: we derive it from the two via get_head_mri_trans()

This to me deviates from the standard processing stream. The fiducials in MRI coordinates do not necessarily have to match exactly (i.e., be apply_trans(mri_fids, mri_head_t) equivalent to) those from head digitization. In fact in general they shouldn't match exactly because head digitization is always off by some mm. Moreover, doing a fit_matched_points between these will be in general worse than a manual (or automated / ICP-based) alignment between the points.

I assume in practice what happens is that some manual or ICP trans is created, then the mri_fiducials update/overwritten with the head_dig transformed to MRI space using the trans, but ...

Currently (i.e., current MNE-BIDS workflow!) these two sets of points implicitly determine the transformation matrix... Does my problem become a little clearer?

... to me this is still a problem with MNE-BIDS deviating from the standard coreg workflow, rather than a problem with MNE-Python. I don't know why the decision was made to implicitly rather than explicitly store the trans (?), but in every dataset I've worked with, the subject/bem/subject-fiducials.fif has never 100% matched the locations obtained by apply_trans(head_fids, head_mri_t), and to me this is a feature not a bug. Forcing it to be the case by (re)positioning the MRI fiducials based on head_fids and trans seems like a less than ideal approach to me, as you've lost information that way.

I'd rather not make it easier for people (in MNE-Python at least) to take the digitized fiducials and transform them to MRI space using a supplied trans, because it gives the false impression that this is what MRI fiducials should be, when really they should be independent, more accurately localized positions of the fiducials.

If it's too late to find a different way to store the trans (e.g., multiple MRI fiducial point definitions, one of which is the accurate/manual set, and the other of which is the current/overwritten/derived-via-trans set?), then to satisfy your use case, MNE-BIDS should just write the subject/bem/subject-fiducials.fif. But this is not a change that needs to be made at the MNE-Python end...

@hoechenberger
Copy link
Member Author

Thanks for the explanation, @larsoner!

I assume in practice what happens is that some manual or ICP trans is created, then the mri_fiducials update/overwritten with the head_dig transformed to MRI space using the trans, but ...

Yes, this is exactly what we're doing

MNE-BIDS should just write the subject/bem/subject-fiducials.fif.

It appears we're actually hitting a limitation of the BIDS specification here, then. We could add a convenience function to MNE-BIDS to create -fiducials.fif based on the MRI metadata. Still, storing a transformation matrix in BIDS is, to my knowledge, not possible.

Bummer.

@larsoner
Copy link
Member

larsoner commented Jan 17, 2022

It appears we're actually hitting a limitation of the BIDS specification here, then.

Indeed -- to add to what you wrote above, just to summarize:

[mne coreg] asks me to place the fiducial points on the MRI[, and I do it and click "save", but MNE-BIDS does not provide a mechanism for saving these points -- so this information is lost when BIDS-ifying.]

Indeed it sounds like there is no way to store this additional information of the manually-specified MRI fiducials (independent of any digitization procedure), and no way to store trans directly -- so you have to lose information. (There is actually a third choice of what information to lose, which would be to overwrite the digitized fids with the MRI ones transformed to head coords, but I don't see this as being any better.)

We could add a convenience function to MNE-BIDS to create -fiducials.fif based on the MRI metadata.

I'm not sure this solves the problem -- if your goal is to allow you really want your collaborator to be able to check/recreate your experience, I think you need to give them your manually created -fiducials.fif.

If you don't care about this loss of information, then we should just make sure at the MNE-Python end that doing coregistration(..., mri_fiducials='mni') will spin up a GUI without asking for clicking on "lock" (if it's explicitly 'mni' rather than 'auto', then I think the user is saying "use these, don't make me reposition them").

@hoechenberger
Copy link
Member Author

I may have found a way to store the information in a BIDS-conforming way:

Screen Shot 2022-01-17 at 21 54 51

https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/02-magnetoencephalography.html#coordinate-system-json-_coordsystemjson

… so I'm thinking we could store both the "true" coordinates as well as those derived by applying the head<>MRI transform in the anatomical metadata.

This would only require some (small-ish) adjustments in MNE-BIDS.

@larsoner
Copy link
Member

… so I'm thinking we could store both the "true" coordinates as well as those derived by applying the head<>MRI transform in the anatomical metadata.

Great!

@hoechenberger
Copy link
Member Author

Let's keep this open until I can confirm that my idea actually works ;)

@hoechenberger
Copy link
Member Author

hoechenberger commented Jan 19, 2022

There should be a step 4.5 here where you click the "save" button. In other words, your manually-clicked-on-the-dense-head-surface MRI fiducial points should be the most accurate ones available, EDIT: and they should be saved to subject/bem/subject-fiducials.fif when you click "save". And importantly, they are independent of the digitization (and any associated error) that was done during the MEG data acquisition.

I only see a single Save button in the GUI of main, and it's grouped under Transform, and the tooltip says it saves the trans. Are you saying this saves both the trans and fiducials?

To quote the long-term user of MNE, @SophieHerbst: "I didn't know you could even save those fiducials." Exact same goes for me, and I've actually been getting paid to work on MNE for almost 2 years straight now … Call me a bad performer, but I believe we really need to rework this behavior of the coreg UI, it's not at all obvious, even to experienced users.

@SophieHerbst
Copy link
Contributor

SophieHerbst commented Jan 19, 2022

Just to clarify, which functions would use these saved mri fiducials other than the co-reg gui?
Wouldn't it make sense to bring up the plot and ask the user to adjust them when writing the BIDS dataset?
This would also solve the problem of working with defaced MRIs after.

@larsoner
Copy link
Member

plot_alignment also uses them if you do mri_fiducials=True IIRC

There was a (fairly visible and in the same space as the lpa/Marion/rpa radio buttons) button to save in the old Mayavi/Traits user interface, perhaps it's missing from the new one

@hoechenberger
Copy link
Member Author

perhaps it's missing from the new one

Ah! This could explain it. Yes it's definitely not there! :)

@GuillaumeFavelier
Copy link
Contributor

I updated #8833 with the feature request:

  • add a button to save the fiducials

@hoechenberger
Copy link
Member Author

@GuillaumeFavelier Looking at the code, I believe the fiducials file is currently saved automatically once the fiducials have been placed, is this correct? If yes, we should discuss whether we'd even want a save button.

@larsoner
Copy link
Member

@GuillaumeFavelier Looking at the code, I believe the fiducials file is currently saved automatically once the fiducials have been placed, is this correct?

When I've run mne coreg multiple times with -s sample, it goes into fiducial-editing mode on startup every time. So it either isn't saving it (to the right place), or it's not detecting the presence of sample/bem/sample-fiducials.fif.

FWIW I don't think it should auto-save -- I think there should be a save button, the filename field should auto-populate with $SUBJECTS_DIR/$SUBJECT/bem/$SUBJECT-fiducials.fif, and if you click "save" and the file exists, you should get a popup prompt asking if you're sure you want to overwrite. (This is one case where a file browser could potentially be used, but we really want people to stick with the standard filename so I'd rather not tempt them to use something else by making browsing/selecting something different easier.)

@hoechenberger
Copy link
Member Author

@larsoner I guess you're right, we should have a button and not auto-save in any case.

@GuillaumeFavelier
Copy link
Contributor

Looking at the code, I believe the fiducials file is currently saved automatically

Hm... What function are you refering to? At the moment, the only function saving fiducials should be _save_subject() which is called only in scaling mode pressing the "Save scaled geometry" button. If you found something else, I think we have to fix this 😅

When I've run mne coreg multiple times with -s sample, it goes into fiducial-editing mode on startup every time. So it either isn't saving it (to the right place), or it's not detecting the presence of sample/bem/sample-fiducials.fif

We do not have a way to specify the path to load the fiducials in the command line with mne coreg right? I know it is possible interactively with the "Load" button in the "MRI fiducials" section or programmatically using the fiducials parameter of the Coregistration/CoregistrationUI class.

we should have a button and not auto-save in any case

Anyway, I already added the feature request/suggestion to #8833 so I'll look into it. Considering the issue is open, I'll give it higher priority then 👍

@larsoner
Copy link
Member

We do not have a way to specify the path to load the fiducials in the command line with mne coreg right?

Correct, and YAGNI. The standard location SUBJECTS_DIR/SUBJECT/bem/SUBJECT-fiducials.fif should be enough in 99% of cases.

@larsoner
Copy link
Member

I'm closing since I think we've taken care of the points in this issue, but feel free to reopen if I'm mistaken @hoechenberger

@hoechenberger
Copy link
Member Author

All good, thanks @larsoner

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

No branches or pull requests

4 participants