[MRG; ENH] Add "add_estimated_fiducials" to DigMontage #9118

adam2392 · 2021-03-16T21:24:47Z

Reference issue

Closes: #9111

Will enhance: #9087 and plot_seeg related tutorials in the future. Necessary ingredients for dataset though:

FreeSurfer subject-specific files
electrode coordinates in FS subject-specific T1w

What does this implement/fix?

Add add_estimated_fiducials function to DigMontage.

Implements unit test... todo

Additional information

From BIDS convos of mne sprint chat.

adam2392 · 2021-03-16T21:25:59Z

@larsoner I am not sure how to best follow the unit tests you specified in #9111 (comment)

I kind of... pseudocoded it out here, but not familiar w/ all the testing datasets in MNE. Would you be able to provide some more explicit guidance here on what exactly do I load and compare?

larsoner · 2021-03-16T21:41:04Z

Probably something close to this:

https://github.com/mne-tools/mne-python/blob/main/mne/tests/test_coreg.py#L267-L281

You want to load the actual manually-created sample -trans.fif, and compare it to the one you get if you use your new function. You can do this second part for example by creating an arbitrary 1-channel montage for sample (the channel position does not matter) with the MRI coordinate frame, then use your estimation function, and montage.get_native_head_t() and see if the rotation angle (quat_to_rot) is small and the translation is small.

mne/channels/montage.py

rob-luke · 2021-03-17T06:19:33Z

The failing ci is due to the doc strings. I made some suggestions about how this could be fixed, there may well be a better fix.

mne/channels/montage.py

larsoner · 2021-03-17T11:27:07Z

mne/channels/montage.py

+        _validate_type(item=fids_mri, types=list, item_name='dig')
+
+        # add those digpoints to montage
+        self.dig.extend(fids_mri)


Might be better to insert these at the beginning, I think that's usually where we put fiducials in the dig list.

mne/channels/tests/test_montage.py

mne/channels/montage.py

Co-authored-by: Robert Luke <748691+rob-luke@users.noreply.github.com>

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

adam2392 · 2021-03-17T13:48:44Z

Is there a way to link the docstring to an existing tutorial?

E.g.

https://mne.tools/stable/auto_tutorials/source-modeling/plot_source_alignment.html?highlight=freesurfer%20coordinates#coordinate-frame-definitions

If a user isn't understanding why one needs the mri coord_frame. I know I asked that question until I read Eric's comment.

adam2392 · 2021-03-17T14:27:40Z

https://26055-1301584-gh.circle-artifacts.com/0/dev/generated/mne.channels.DigMontage.html#mne.channels.DigMontage.add_estimated_fiducials

drammock · 2021-03-17T14:52:39Z

Is there a way to link the docstring to an existing tutorial?

Crossrefs to tutorials are probably most appropriate in a See also section, but could also be added inline to the docstring intro paragraph. To link to that tutorial you can use its target name:

mne-python/tutorials/source-modeling/plot_source_alignment.py

Line 3 in 1026ad7

.. _plot_source_alignment:

adam2392 · 2021-03-17T16:19:29Z

If this CI works, will add the changelog. don't wanna hit the CIs again :p

larsoner · 2021-03-17T16:25:42Z

Feel free to push, I'll probably go through and clear out the GitHub Actions log soon anyway...

larsoner

LGTM +1 for merge once CIs come back happy (someday)

mne/channels/montage.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

adam2392 · 2021-03-17T20:28:30Z

https://26126-1301584-gh.circle-artifacts.com/0/dev/generated/mne.channels.DigMontage.html#mne.channels.DigMontage.add_estimated_fiducials

larsoner · 2021-03-18T14:26:17Z

Thanks @adam2392 !

agramfort

sorry for coming late to the party :(

agramfort · 2021-03-18T14:37:32Z

doc/changes/latest.inc

- :func:`mne.io.anonymize_info` now anonymizes also sex and hand fields when ``keep_his`` is ``False`` (:gh:`9103` by |Rotem Falach|_)`
+- Add :func:`mne.channels.DigMontage.add_estimated_fiducials` which will add LPA, RPA and Nasion fiducial points to the ``DigMontage`` object in ``mri`` coordinate frame (:gh:`9118` by `Adam Li`_)
+
+  - :func:`mne.io.anonymize_info` now anonymizes also sex and hand fields when ``keep_his`` is ``False`` (:gh:`9103` by |Rotem Falach|_)`


I fixed this in main, it was my error during the mege...

agramfort · 2021-03-18T14:38:21Z

mne/channels/montage.py

+        This takes a montage with the ``mri`` coordinate frame,
+        corresponding to the FreeSurfer RAS (xyz in the volume) T1w
+        image of the specific subject. It will call
+        :func:`mne.coreg.get_mni_fiducials` to estimate LPA, RPA and


I don't like to have stuff from coreg module to be imported outside. coreg should be GUI code only I think.

Agreed we could move it to source_space.py, it's where we deal with coordinate frames most of the time anyway

agramfort · 2021-03-18T14:38:42Z

mne/channels/montage.py

+        and then use ``montage.get_native_head_t()`` to get the
+        head <-> MRI transform.
+        """
+        from ..coreg import get_mni_fiducials


yes nested import is required exactly for this I think.

agramfort · 2021-03-19T10:59:35Z

Agreed we could move it to source_space.py, it's where we deal with coordinate frames most of the time anyway

+1 can you do the PR?

Adding updated unit test.

6aa4014

adam2392 added 2 commits March 16, 2021 21:14

Fixing unit test.

2210ba7

Fixing flake.

17b6f18

rob-luke reviewed Mar 17, 2021

View reviewed changes

mne/channels/montage.py Outdated Show resolved Hide resolved

rob-luke reviewed Mar 17, 2021

View reviewed changes

mne/channels/montage.py Outdated Show resolved Hide resolved

rob-luke reviewed Mar 17, 2021

View reviewed changes

mne/channels/montage.py Show resolved Hide resolved

larsoner reviewed Mar 17, 2021

View reviewed changes

adam2392 and others added 7 commits March 17, 2021 09:30

Update mne/channels/montage.py

2d1c788

Co-authored-by: Robert Luke <748691+rob-luke@users.noreply.github.com>

Update mne/channels/montage.py

5a7be98

Co-authored-by: Robert Luke <748691+rob-luke@users.noreply.github.com>

Update mne/channels/montage.py

ed1c656

Co-authored-by: Robert Luke <748691+rob-luke@users.noreply.github.com>

Update mne/channels/montage.py

0d6bfbc

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Update mne/channels/tests/test_montage.py

0cd1287

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Address mne comments.

c69d654

Adding updated docstring.

ba60b47

Adding docstring for test montage.

706147c

adam2392 added 2 commits March 17, 2021 12:06

ADding crossref.

8aa5264

Adding changelog.

d3c3c6b

adam2392 added 3 commits March 17, 2021 13:51

Fix order of docstring.

9553988

Reference tutorial.'

e587a07

Does this work.

6ba5757

adam2392 requested review from rob-luke and larsoner March 17, 2021 19:40

adam2392 changed the title ~~[WIP] Add "add_estimated_fiducials" to DigMontage~~ [MRG; ENH] Add "add_estimated_fiducials" to DigMontage Mar 17, 2021

adam2392 mentioned this pull request Mar 17, 2021

Writing/Reading iEEG Coordinate Frames - FreeSurfer volume and FreeSurfer RAS mne-tools/mne-bids#737

Open

larsoner approved these changes Mar 17, 2021

View reviewed changes

larsoner reviewed Mar 17, 2021

View reviewed changes

mne/channels/montage.py Outdated Show resolved Hide resolved

adam2392 and others added 2 commits March 17, 2021 16:08

Update mne/channels/montage.py

07b13ed

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

BUG: Link

dda5312

larsoner approved these changes Mar 17, 2021

View reviewed changes

adam2392 and others added 2 commits March 17, 2021 18:03

Fixing a docstring.g

48f9771

Merge branch 'main' into estfids

9aa6f30

larsoner merged commit ee18776 into mne-tools:main Mar 18, 2021

agramfort reviewed Mar 18, 2021

View reviewed changes

larsoner mentioned this pull request Mar 19, 2021

MAINT: Move function #9165

Merged

adam2392 deleted the estfids branch August 25, 2021 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG; ENH] Add "add_estimated_fiducials" to DigMontage #9118

[MRG; ENH] Add "add_estimated_fiducials" to DigMontage #9118

adam2392 commented Mar 16, 2021 •

edited

adam2392 commented Mar 16, 2021

larsoner commented Mar 16, 2021

rob-luke commented Mar 17, 2021

larsoner Mar 17, 2021

adam2392 commented Mar 17, 2021

adam2392 commented Mar 17, 2021

drammock commented Mar 17, 2021

adam2392 commented Mar 17, 2021

larsoner commented Mar 17, 2021

larsoner left a comment

adam2392 commented Mar 17, 2021

larsoner commented Mar 18, 2021

agramfort left a comment

agramfort Mar 18, 2021

larsoner Mar 18, 2021

agramfort Mar 18, 2021

larsoner Mar 18, 2021

agramfort Mar 18, 2021

agramfort commented Mar 19, 2021 via email

[MRG; ENH] Add "add_estimated_fiducials" to DigMontage #9118

[MRG; ENH] Add "add_estimated_fiducials" to DigMontage #9118

Conversation

adam2392 commented Mar 16, 2021 • edited

Reference issue

What does this implement/fix?

Additional information

adam2392 commented Mar 16, 2021

larsoner commented Mar 16, 2021

rob-luke commented Mar 17, 2021

larsoner Mar 17, 2021

Choose a reason for hiding this comment

adam2392 commented Mar 17, 2021

adam2392 commented Mar 17, 2021

drammock commented Mar 17, 2021

adam2392 commented Mar 17, 2021

larsoner commented Mar 17, 2021

larsoner left a comment

Choose a reason for hiding this comment

adam2392 commented Mar 17, 2021

larsoner commented Mar 18, 2021

agramfort left a comment

Choose a reason for hiding this comment

agramfort Mar 18, 2021

Choose a reason for hiding this comment

larsoner Mar 18, 2021

Choose a reason for hiding this comment

agramfort Mar 18, 2021

Choose a reason for hiding this comment

larsoner Mar 18, 2021

Choose a reason for hiding this comment

agramfort Mar 18, 2021

Choose a reason for hiding this comment

agramfort commented Mar 19, 2021 via email

adam2392 commented Mar 16, 2021 •

edited