BUG: Fix bug with coord frame reading (#13129)

Author: larsoner

mne/_fiff/_digitization.py

@@ -186,7 +186,8 @@ def _read_dig_fif(fid, meas_info, *, return_ch_names=False):
                 dig.extend(tag.data)
             elif kind == FIFF.FIFF_MNE_COORD_FRAME:
                 tag = read_tag(fid, pos)
-                coord_frame = _coord_frame_named.get(int(tag.data.item()))
+                coord_frame = int(tag.data.item())
+                coord_frame = _coord_frame_named.get(coord_frame, coord_frame)
             elif kind == FIFF.FIFF_MNE_CH_NAME_LIST:
                 tag = read_tag(fid, pos)
                 ch_names = _safe_name_list(tag.data, "read", "ch_names")

mne/_fiff/constants.py

@@ -281,10 +281,11 @@
         FIFF.FIFFV_COORD_HPI,
         FIFF.FIFFV_COORD_HEAD,
         FIFF.FIFFV_COORD_MRI,
-        FIFF.FIFFV_COORD_MRI_SLICE,
-        FIFF.FIFFV_COORD_MRI_DISPLAY,
-        FIFF.FIFFV_COORD_DICOM_DEVICE,
-        FIFF.FIFFV_COORD_IMAGING_DEVICE,
+        # We never use these but could add at some point
+        # FIFF.FIFFV_COORD_MRI_SLICE,
+        # FIFF.FIFFV_COORD_MRI_DISPLAY,
+        # FIFF.FIFFV_COORD_DICOM_DEVICE,
+        # FIFF.FIFFV_COORD_IMAGING_DEVICE,
     )
 }
 #
@@ -817,6 +818,17 @@
 #
 FIFF.FIFFV_MNE_COORD_4D_HEAD = FIFF.FIFFV_MNE_COORD_CTF_HEAD
 FIFF.FIFFV_MNE_COORD_KIT_HEAD = FIFF.FIFFV_MNE_COORD_CTF_HEAD
+_coord_frame_named.update({
+    key: key
+    for key in (
+        FIFF.FIFFV_MNE_COORD_CTF_DEVICE,
+        FIFF.FIFFV_MNE_COORD_MRI_VOXEL,
+        FIFF.FIFFV_MNE_COORD_RAS,
+        FIFF.FIFFV_MNE_COORD_MNI_TAL,
+        FIFF.FIFFV_MNE_COORD_FS_TAL,
+        FIFF.FIFFV_MNE_COORD_KIT_HEAD,
+    )
+})
 
 #
 #   FWD Types

mne/_fiff/tests/test_constants.py

@@ -23,6 +23,7 @@
     _dig_kind_named,
 )
 from mne.forward._make_forward import _read_coil_defs
+from mne.transforms import _frame_to_str, _verbose_frames
 from mne.utils import requires_good_network
 
 # https://github.com/mne-tools/fiff-constants/commits/master
@@ -405,7 +406,14 @@ def test_constants(tmp_path):
             "^FIFFV_.*_CH$",
             (FIFF.FIFFV_DIPOLE_WAVE, FIFF.FIFFV_GOODNESS_FIT),
         ),
-        (_coord_frame_named, "FIFFV_COORD_", ()),
+        pytest.param(
+            _coord_frame_named,
+            "FIFFV_(MNE_)?COORD_",
+            (),
+            marks=pytest.mark.xfail(
+                reason="Intentional mismatch tested by test_coord_frame_consistency",
+            ),
+        ),
         (_ch_unit_named, "FIFF_UNIT_", ()),
         (_ch_unit_mul_named, "FIFF_UNITM_", ()),
         (_ch_coil_type_named, "FIFFV_COIL_", ()),
@@ -419,3 +427,29 @@ def test_dict_completion(dict_, match, extras):
         got.add(e)
     want = set(dict_)
     assert got == want, match
+
+
+def test_coord_frame_consistency():
+    """Test consistency between coord frame mappings."""
+    all_frames = set(
+        key for key in dir(FIFF) if key.startswith(("FIFFV_COORD_", "FIFFV_MNE_COORD"))
+    )
+    # ... but there are some frames that we never work in so let's cull those for now
+    ignore_frames = set(
+        f"FIFFV_COORD_{name}"
+        for name in """
+        MRI_SLICE MRI_DISPLAY DICOM_DEVICE IMAGING_DEVICE
+        """.strip().split()
+    )
+    ignore_frames |= set(
+        f"FIFFV_MNE_COORD_{name}"
+        for name in """
+        DIGITIZER TUFTS_EEG FS_TAL_GTZ FS_TAL_LTZ
+        """.strip().split()
+    )
+    assert ignore_frames.issubset(all_frames)
+    all_frames -= ignore_frames
+    all_ints = set(FIFF[key] for key in all_frames)
+    assert set(_frame_to_str) == all_ints
+    assert set(_verbose_frames) == all_ints
+    assert set(_coord_frame_named) == all_ints

mne/channels/tests/test_montage.py

@@ -151,22 +151,35 @@ def test_dig_montage_trans(tmp_path):
     assert str(position1) == str(position2)  # exactly equal
 
 
-def test_fiducials():
+@pytest.mark.parametrize("fname", (fif_fname, ctf_fif_fname))
+def test_fiducials(tmp_path, fname):
     """Test handling of fiducials."""
     # Eventually the code used here should be unified with montage.py, but for
     # now it uses code in odd places
-    for fname in (fif_fname, ctf_fif_fname):
-        fids, coord_frame = read_fiducials(fname)
-        points = _fiducial_coords(fids, coord_frame)
-        assert points.shape == (3, 3)
-        # Fids
-        assert_allclose(points[:, 2], 0.0, atol=1e-6)
-        assert_allclose(points[::2, 1], 0.0, atol=1e-6)
-        assert points[2, 0] > 0  # RPA
-        assert points[0, 0] < 0  # LPA
-        # Nasion
-        assert_allclose(points[1, 0], 0.0, atol=1e-6)
-        assert points[1, 1] > 0
+    fids, coord_frame = read_fiducials(fname)
+    assert coord_frame == FIFF.FIFFV_COORD_HEAD
+    points = _fiducial_coords(fids, coord_frame)
+    assert points.shape == (3, 3)
+    # Fids
+    assert_allclose(points[:, 2], 0.0, atol=1e-6)
+    assert_allclose(points[::2, 1], 0.0, atol=1e-6)
+    assert points[2, 0] > 0  # RPA
+    assert points[0, 0] < 0  # LPA
+    # Nasion
+    assert_allclose(points[1, 0], 0.0, atol=1e-6)
+    assert points[1, 1] > 0
+    fname_out = tmp_path / "test-dig.fif"
+    make_dig_montage(
+        lpa=fids[0]["r"], nasion=fids[1]["r"], rpa=fids[2]["r"], coord_frame="mri_voxel"
+    ).save(fname_out, overwrite=True)
+    fids_2, coord_frame_2 = read_fiducials(fname_out)
+    assert coord_frame_2 == FIFF.FIFFV_MNE_COORD_MRI_VOXEL
+    assert_allclose(
+        [fid["r"] for fid in fids[:3]],
+        [fid["r"] for fid in fids_2],
+        rtol=1e-6,
+    )
+    assert coord_frame_2 is not None
 
 
 def test_documented():

mne/transforms.py

@@ -42,10 +42,12 @@
 
 
 _str_to_frame = dict(
+    isotrak=FIFF.FIFFV_COORD_ISOTRAK,
     meg=FIFF.FIFFV_COORD_DEVICE,
     mri=FIFF.FIFFV_COORD_MRI,
     mri_voxel=FIFF.FIFFV_MNE_COORD_MRI_VOXEL,
     head=FIFF.FIFFV_COORD_HEAD,
+    hpi=FIFF.FIFFV_COORD_HPI,
     mni_tal=FIFF.FIFFV_MNE_COORD_MNI_TAL,
     ras=FIFF.FIFFV_MNE_COORD_RAS,
     fs_tal=FIFF.FIFFV_MNE_COORD_FS_TAL,
@@ -63,15 +65,16 @@
     FIFF.FIFFV_COORD_HEAD: "head",
     FIFF.FIFFV_COORD_MRI: "MRI (surface RAS)",
     FIFF.FIFFV_MNE_COORD_MRI_VOXEL: "MRI voxel",
-    FIFF.FIFFV_COORD_MRI_SLICE: "MRI slice",
-    FIFF.FIFFV_COORD_MRI_DISPLAY: "MRI display",
     FIFF.FIFFV_MNE_COORD_CTF_DEVICE: "CTF MEG device",
     FIFF.FIFFV_MNE_COORD_CTF_HEAD: "CTF/4D/KIT head",
     FIFF.FIFFV_MNE_COORD_RAS: "RAS (non-zero origin)",
     FIFF.FIFFV_MNE_COORD_MNI_TAL: "MNI Talairach",
-    FIFF.FIFFV_MNE_COORD_FS_TAL_GTZ: "Talairach (MNI z > 0)",
-    FIFF.FIFFV_MNE_COORD_FS_TAL_LTZ: "Talairach (MNI z < 0)",
-    -1: "unknown",
+    FIFF.FIFFV_MNE_COORD_FS_TAL: "FS Talairach",
+    # We don't use these, but keep them in case we ever want to add them.
+    # FIFF.FIFFV_COORD_MRI_SLICE: "MRI slice",
+    # FIFF.FIFFV_COORD_MRI_DISPLAY: "MRI display",
+    # FIFF.FIFFV_MNE_COORD_FS_TAL_GTZ: "Talairach (MNI z > 0)",
+    # FIFF.FIFFV_MNE_COORD_FS_TAL_LTZ: "Talairach (MNI z < 0)",
 }