MRG: Make update_anat_landmarks() accept fiducials file

[hoechenberger]

Fixes #968

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]

Codecov Report

Merging #977 (65c45a1) into main (b26f062) will decrease coverage by 0.01%.
The diff coverage is 95.00%.

❗ Current head 65c45a1 differs from pull request most recent head aaa3f45. Consider uploading reports for the commit aaa3f45 to get more accurate results

@@            Coverage Diff             @@
##             main     #977      +/-   ##
==========================================
- Coverage   94.63%   94.62%   -0.02%     
==========================================
  Files          25       25              
  Lines        3600     3626      +26     
==========================================
+ Hits         3407     3431      +24     
- Misses        193      195       +2     

Impacted Files	Coverage Δ
mne_bids/read.py	95.91% <0.00%> (-0.25%)	⬇️
mne_bids/sidecar_updates.py	98.09% <95.83%> (-0.72%)	⬇️
mne_bids/write.py	96.62% <100.00%> (+0.01%)	⬆️

---

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 a16a1c7...aaa3f45. Read the comment docs.

[hoechenberger]

@agramfort Turns out we didn't have an issue with zooming … I just used sample fiducials on testing-data MRI 🤦

[hoechenberger]

@agramfort I've addressed all your comments, this should be good to merge once CI comes back green

[agramfort]

ok to merge this as is as it already fixes a practical problem but it seems the code only works if the landmarks passed as DigMontage are already in mri voxels. This should not be needed. We allow the same thing as what we allow with write_anat

@hoechenberger if you want we can merge this and please create an issue for the next stuff to do

[hoechenberger]

@agramfort

ok to merge this as is as it already fixes a practical problem but it seems the code only works if the landmarks passed as DigMontage are already in mri voxels. This should not be needed. We allow the same thing as what we allow with write_anat

Very interesting observation, I just looked at the docstring and implementation of write_anat(); it calls _get_landmarks(), which only works with coordinates in scanner RAS and voxel space (and raises an exception otherwise).

The docstring suggests to use get_anat_landmarks() to produce landmark coordinates in the required coordinate frame (the function returns coords in voxel space); scanner RAS is never mentioned anywhere in the write_anat() docstring.

So this is at least a documentation bug / shortcoming.

But it also means that update_anat_landmarks() should support scanner RAS coordinates as well, in addition to voxel (which is already supported in main) and mri (surface RAS; for which this PR adds support)

I will look into this.

[hoechenberger]

@agramfort I've decided to document the inconsistency in #978 and will go ahead here and merge for now!