Deprecate parts of read_dig_montage (#6712)

massich · agramfort · commit 3e60a9a45c2e · 2019-08-27T10:57:30.000+02:00
* WIP: deprecate FIF, and let codecov reveal the dead code.

* WIP: deprecate EGI and branvision in `read_dig_montage`

* WIP: add alternative to the warning msg

* wip

* WIP: remove dead code

* ups

* forgot whatsnew
diff --git a/doc/changes/latest.inc b/doc/changes/latest.inc
@@ -131,6 +131,10 @@ Bug
 API
 ~~~
 
+- Deprecate passing ``np.arrays`` as ``hsp``, ``hpi`` or ``elp`` parameters to ``mne.channels.read_dig_montage`` by `Joan Massich`_.
+
+- Deprecate ``fif`` and ``egi`` parameters in ``mne.channels.read_dig_montage`` by `Joan Massich`_.
+
 - Deprecate ``mne.evoked.grand_average`` in favor of :func:`mne.grand_average` (which works on both :class:`~mne.Evoked` and :class:`~mne.time_frequency.AverageTFR`) by `Daniel McCloy`_
 
 - Deprecate ``exclude`` parameter in :func:`mne.viz.plot_ica_sources` and :meth:`mne.preprocessing.ICA.plot_sources`, instead always use the ``exclude`` attribute of the ICA object by `Daniel McCloy`_.
diff --git a/mne/channels/_dig_montage_utils.py b/mne/channels/_dig_montage_utils.py
@@ -18,8 +18,6 @@
 from ..transforms import apply_trans, get_ras_to_neuromag_trans
 
 from ..io.constants import FIFF
-from ..io.open import fiff_open
-from .._digitization._utils import _read_dig_fif
 from ..utils import _check_fname, Bunch, warn
 
 
@@ -96,60 +94,6 @@ def _transform_to_head_call(data):
 }
 
 
-def _read_dig_montage_fif(
-        fname,
-        _raise_transform_err,
-        _all_data_kwargs_are_none,
-):
-    from .montage import _check_frame   # circular dep
-
-    if _raise_transform_err:
-        raise ValueError('transform must be True and dev_head_t must be '
-                         'False for FIF dig montage')
-    if not _all_data_kwargs_are_none:
-        raise ValueError('hsp, hpi, elp, point_names, egi must all be '
-                         'None if fif is not None')
-
-    _check_fname(fname, overwrite='read', must_exist=True)
-    # Load the dig data
-    f, tree = fiff_open(fname)[:2]
-    with f as fid:
-        dig = _read_dig_fif(fid, tree)
-
-    # Split up the dig points by category
-    hsp = list()
-    hpi = list()
-    elp = list()
-    point_names = list()
-    fids = dict()
-    dig_ch_pos = dict()
-    for d in dig:
-        if d['kind'] == FIFF.FIFFV_POINT_CARDINAL:
-            _check_frame(d, 'head')
-            fids[_cardinal_ident_mapping[d['ident']]] = d['r']
-        elif d['kind'] == FIFF.FIFFV_POINT_HPI:
-            _check_frame(d, 'head')
-            hpi.append(d['r'])
-            elp.append(d['r'])
-            point_names.append('HPI%03d' % d['ident'])
-        elif d['kind'] == FIFF.FIFFV_POINT_EXTRA:
-            _check_frame(d, 'head')
-            hsp.append(d['r'])
-        elif d['kind'] == FIFF.FIFFV_POINT_EEG:
-            _check_frame(d, 'head')
-            dig_ch_pos['EEG%03d' % d['ident']] = d['r']
-
-    return Bunch(
-        nasion=fids['nasion'], lpa=fids['lpa'], rpa=fids['rpa'],
-        hsp=np.array(hsp) if len(hsp) else None,
-        hpi=hpi,  # XXX: why this is returned as empty list instead of None?
-        elp=np.array(elp) if len(elp) else None,
-        point_names=point_names,
-        dig_ch_pos=dig_ch_pos,
-        coord_frame='head',
-    )
-
-
 def _read_dig_montage_egi(
         fname,
         _scaling,
diff --git a/mne/channels/montage.py b/mne/channels/montage.py
@@ -22,7 +22,7 @@
 from ..viz import plot_montage
 from .channels import _contains_ch_type
 from ..transforms import (apply_trans, get_ras_to_neuromag_trans, _sph_to_cart,
-                          _topo_to_sph, _str_to_frame, _frame_to_str)
+                          _topo_to_sph, _frame_to_str)
 from .._digitization import Digitization
 from .._digitization._utils import (_make_dig_points, _read_dig_points,
                                     write_dig, _read_dig_fif)
@@ -34,7 +34,7 @@
                      _check_fname)
 
 from .layout import _pol_to_cart, _cart_to_sph
-from ._dig_montage_utils import _transform_to_head_call, _read_dig_montage_fif
+from ._dig_montage_utils import _transform_to_head_call
 from ._dig_montage_utils import _read_dig_montage_egi, _read_dig_montage_bvct
 from ._dig_montage_utils import _foo_get_data_from_dig
 from ._dig_montage_utils import _fix_data_fiducials
@@ -764,13 +764,6 @@ def nasion(self):
         return _foo_get_data_from_dig(self.dig).nasion
 
 
-def _check_frame(d, frame_str):
-    """Check coordinate frames."""
-    if d['coord_frame'] != _str_to_frame[frame_str]:
-        raise RuntimeError('dig point must be in %s coordinate frame, got %s'
-                           % (frame_str, _frame_to_str[d['coord_frame']]))
-
-
 def _get_scaling(unit, scale):
     if unit not in scale:
         raise ValueError("Unit needs to be one of %s, not %r" %
@@ -821,6 +814,41 @@ def transform_to_head(montage):
     return montage
 
 
+def _read_dig_montage_deprecation_warning_helper(**kwargs):
+    if kwargs.pop('fif') is not None:
+        warn('Using "read_dig_montage" with "fif" not None'
+             ' is deprecated and will be removed in v0.20'
+             ' Please use read_dig_fif instead.', DeprecationWarning)
+        return
+    if kwargs.pop('egi') is not None:
+        warn('Using "read_dig_montage" with "egi" not None'
+             ' is deprecated and will be removed in v0.20'
+             ' Please use read_dig_egi instead.', DeprecationWarning)
+        return
+    if kwargs.pop('bvct') is not None:
+        warn('Using "read_dig_montage" with "bvct" not None'
+             ' is deprecated and will be removed in v0.20.'
+             ' Please use read_dig_captrack instead.', DeprecationWarning)
+        return
+
+    # XXX: for now we only have implemented the case where hsp, hpi and elp
+    #      are np.arrays. we need read_dig_polhemus for the str cases. Plus,
+    #      we need to make sure that we can handle a mix of arrays and str. The
+    #      mixed case is not in our code-base but there was nothing preventing
+    #      a user to do so.
+    #
+    #      we can assume that whatever is not None or np.array is path-like
+    #      therefore str.
+    if [kk for kk in ('hsp', 'hpi', 'elp') if isinstance(kwargs[kk],
+                                                         np.ndarray)]:
+        warn('Passing "np.arrays" to "hsp", "hpi" or "elp" in'
+             ' "read_dig_montage" is deprecated and will be removed in v0.20.'
+             ' Please use "make_dig_montage" instead.', DeprecationWarning)
+        return
+
+    pass  # noqa  # XXX: will grow with issue-6461
+
+
 def read_dig_montage(hsp=None, hpi=None, elp=None,
                      point_names=None, unit='auto', fif=None, egi=None,
                      bvct=None, transform=True, dev_head_t=False, ):
@@ -911,15 +939,25 @@ def read_dig_montage(hsp=None, hpi=None, elp=None,
     EGI_SCALE = dict(mm=1e-3, cm=1e-2, auto=1e-2, m=1)
     NUMPY_DATA_SCALE = dict(mm=1e-3, cm=1e-2, auto=1e-3, m=1)
 
+    _read_dig_montage_deprecation_warning_helper(
+        hsp=hsp, hpi=hpi, elp=elp, fif=fif, egi=egi, bvct=bvct,
+    )
+
     if fif is not None:
         _raise_transform_err = True if dev_head_t or not transform else False
-        data = _read_dig_montage_fif(
-            fname=fif,
-            _raise_transform_err=_raise_transform_err,
-            _all_data_kwargs_are_none=all(
-                x is None for x in (hsp, hpi, elp, point_names, egi, bvct))
+        _all_data_kwargs_are_none = all(
+            x is None for x in (hsp, hpi, elp, point_names, egi, bvct)
         )
 
+        if _raise_transform_err:
+            raise ValueError('transform must be True and dev_head_t must be'
+                             ' False for FIF dig montage')
+        if not _all_data_kwargs_are_none:
+            raise ValueError('hsp, hpi, elp, point_names, egi must all be'
+                             ' None if fif is not None')
+
+        return read_dig_fif(fname=fif)
+
     elif egi is not None:
         data = _read_dig_montage_egi(
             fname=egi,
diff --git a/mne/channels/tests/test_montage.py b/mne/channels/tests/test_montage.py
@@ -407,11 +407,12 @@ def test_read_dig_montage():
     assert_allclose(montage.dev_head_t, EXPECTED_DEV_HEAD_T, atol=1e-7)
 
     # Digitizer as array
-    m2 = read_dig_montage(hsp_points, hpi_points, elp_points, names, unit='m')
     with pytest.deprecated_call():
+        m2 = read_dig_montage(hsp_points, hpi_points, elp_points, names,
+                              unit='m')
         assert_array_equal(m2.hsp, montage.hsp)
-    m3 = read_dig_montage(hsp_points * 1000, hpi_points, elp_points * 1000,
-                          names)
+        m3 = read_dig_montage(hsp_points * 1000, hpi_points, elp_points * 1000,
+                              names)
     with pytest.deprecated_call():
         assert_allclose(m3.hsp, montage.hsp)
 
@@ -460,7 +461,8 @@ def test_set_dig_montage():
     temp_dir = _TempDir()
     fname_temp = op.join(temp_dir, 'test.fif')
     montage.save(fname_temp)
-    montage_read = read_dig_montage(fif=fname_temp)
+    with pytest.deprecated_call():
+        montage_read = read_dig_montage(fif=fname_temp)
     for use_mon in (montage, montage_read):
         info = create_info(['Test Ch'], 1e3, ['eeg'])
         with pytest.warns(None):  # warns on one run about not all positions
@@ -489,7 +491,8 @@ def test_set_dig_montage():
 @testing.requires_testing_data
 def test_fif_dig_montage():
     """Test FIF dig montage support."""
-    dig_montage = read_dig_montage(fif=fif_dig_montage_fname)
+    with pytest.deprecated_call():
+        dig_montage = read_dig_montage(fif=fif_dig_montage_fname)
 
     # test round-trip IO
     temp_dir = _TempDir()
@@ -535,7 +538,8 @@ def test_fif_dig_montage():
     _check_roundtrip(montage, fname_temp)
 
     # Test old way matches new way
-    dig_montage = read_dig_montage(fif=fif_dig_montage_fname)
+    with pytest.deprecated_call():
+        dig_montage = read_dig_montage(fif=fif_dig_montage_fname)
     dig_montage_fif = read_dig_fif(fif_dig_montage_fname)
     assert dig_montage.dig == dig_montage_fif.dig
     assert object_diff(dig_montage.ch_names, dig_montage_fif.ch_names) == ''
@@ -544,7 +548,8 @@ def test_fif_dig_montage():
 @testing.requires_testing_data
 def test_egi_dig_montage():
     """Test EGI MFF XML dig montage support."""
-    dig_montage = read_dig_montage(egi=egi_dig_montage_fname, unit='m')
+    with pytest.deprecated_call():
+        dig_montage = read_dig_montage(egi=egi_dig_montage_fname, unit='m')
 
     # # test round-trip IO
     temp_dir = _TempDir()
@@ -575,7 +580,8 @@ def test_egi_dig_montage():
     assert_dig_allclose(raw_egi.info, test_raw_egi.info)
 
     # Test old way matches new way
-    dig_montage = read_dig_montage(egi=egi_dig_montage_fname, unit='m')
+    with pytest.deprecated_call():
+        dig_montage = read_dig_montage(egi=egi_dig_montage_fname, unit='m')
     dig_montage_egi = read_dig_egi(egi_dig_montage_fname)
     dig_montage_egi = transform_to_head(dig_montage_egi)
     assert dig_montage.dig == dig_montage_egi.dig
@@ -588,7 +594,8 @@ def test_bvct_dig_montage_old_api():  # XXX: to remove in 0.20
     with pytest.warns(RuntimeWarning, match='Using "m" as unit for BVCT file'):
         read_dig_montage(bvct=bvct_dig_montage_fname, unit='m')
 
-    dig_montage = read_dig_montage(bvct=bvct_dig_montage_fname)
+    with pytest.deprecated_call():
+        dig_montage = read_dig_montage(bvct=bvct_dig_montage_fname)
 
     # test round-trip IO
     temp_dir = _TempDir()
@@ -682,7 +689,7 @@ def _check_roundtrip(montage, fname):
     with pytest.deprecated_call():
         assert_equal(montage.coord_frame, 'head')
     montage.save(fname)
-    montage_read = read_dig_montage(fif=fname)
+    montage_read = read_dig_fif(fname=fname)
     assert_equal(str(montage), str(montage_read))
     with pytest.deprecated_call():
         for kind in ('elp', 'hsp', 'nasion', 'lpa', 'rpa'):
diff --git a/mne/gui/_file_traits.py b/mne/gui/_file_traits.py
@@ -29,7 +29,7 @@
                      create_default_subject)
 from ..utils import get_config, set_config
 from ..viz._3d import _fiducial_coords
-from ..channels import read_dig_montage, DigMontage
+from ..channels import read_dig_fif, DigMontage
 
 
 fid_wildcard = "*.fif"
@@ -300,7 +300,7 @@ def _get__info(self):
             if len(dir_tree_find(tree, FIFF.FIFFB_MEAS_INFO)) > 0:
                 info = read_info(self.file, verbose=False)
             elif len(dir_tree_find(tree, FIFF.FIFFB_ISOTRAK)) > 0:
-                info = read_dig_montage(fif=self.file, transform=True)
+                info = read_dig_fif(fname=self.file)
 
             if isinstance(info, DigMontage):
                 dig = info.dig
diff --git a/mne/gui/tests/test_file_traits.py b/mne/gui/tests/test_file_traits.py
@@ -12,7 +12,7 @@
 from mne.datasets import testing
 from mne.io.tests import data_dir as fiff_data_dir
 from mne.utils import _TempDir, requires_mayavi, run_tests_if_main, traits_test
-from mne.channels import read_dig_montage
+from mne.channels import read_dig_fif
 
 data_path = testing.data_path(download=False)
 subjects_dir = op.join(data_path, 'subjects')
@@ -77,7 +77,7 @@ def test_inst_source():
     assert_allclose(inst.nasion, nasion)
     assert_allclose(inst.rpa, rpa)
 
-    montage = read_dig_montage(fif=inst_path)  # test reading DigMontage
+    montage = read_dig_fif(inst_path)  # test reading DigMontage
     montage_path = op.join(tempdir, 'temp_montage.fif')
     montage.save(montage_path)
     inst.file = montage_path
diff --git a/mne/viz/tests/test_montage.py b/mne/viz/tests/test_montage.py
@@ -10,7 +10,7 @@
 import pytest
 import matplotlib.pyplot as plt
 
-from mne.channels import read_montage, read_dig_montage
+from mne.channels import read_montage, read_dig_montage, read_dig_fif
 
 p_dir = op.join(op.dirname(__file__), '..', '..', 'io', 'kit', 'tests', 'data')
 elp = op.join(p_dir, 'test_elp.txt')
@@ -38,7 +38,7 @@ def test_plot_montage():
     assert '0 channels' in repr(d)
     with pytest.raises(RuntimeError, match='No valid channel positions'):
         d.plot()
-    d = read_dig_montage(fif=fif_fname)
+    d = read_dig_fif(fname=fif_fname)
     assert '61 channels' in repr(d)
     # XXX this is broken; dm.point_names is used. Sometimes we say this should
     # Just contain the HPI coils, other times that it's all channels (e.g.,
