Work on fixing issues with multi-file converters.

Author: tsalo

heudiconv/convert.py

@@ -311,21 +311,28 @@ def update_multiecho_name(metadata, filename, echo_times):
     filename : str
         Updated filename with echo entity added, if appropriate.
     """
-    # Field maps separate echoes differently
+    # Field maps separate echoes differently, so do not attempt to update any filenames with these
+    # suffixes
     unsupported_types = [
         '_magnitude', '_magnitude1', '_magnitude2',
         '_phasediff', '_phase1', '_phase2', '_fieldmap'
     ]
     if any(ut in filename for ut in unsupported_types):
         return filename
 
-    assert isinstance(echo_times, list), 'Argument "echo_times" must be a list.'
+    if not isinstance(echo_times, list):
+        raise TypeError(f'Argument "echo_times" must be a list, not a {type(echo_times)}')
 
-    # Get the EchoNumber from json file info.  If not present, use EchoTime
+    # Get the EchoNumber from json file info.  If not present, use EchoTime.
     if 'EchoNumber' in metadata.keys():
         echo_number = metadata['EchoNumber']
-    else:
+    elif 'EchoTime' in metadata.keys():
         echo_number = echo_times.index(metadata['EchoTime']) + 1
+    else:
+        raise KeyError(
+            'Either "EchoNumber" or "EchoTime" must be in metadata keys. '
+            f'Keys detected: {metadata.keys()}'
+        )
 
     # Determine scan suffix
     filetype = '_' + filename.split('_')[-1]
@@ -366,7 +373,8 @@ def update_uncombined_name(metadata, filename, channel_names):
     if any(ut in filename for ut in unsupported_types):
         return filename
 
-    assert isinstance(channel_names, list), 'Argument "channel_names" must be a list.'
+    if not isinstance(channel_names, list):
+        raise TypeError(f'Argument "channel_names" must be a list, not a {type(channel_names)}')
 
     # Determine the channel number
     channel_number = ''.join([c for c in metadata['CoilString'] if c.isdigit()])
@@ -694,9 +702,12 @@ def save_converted_files(res, item_dicoms, bids_options, outtype, prefix, outnam
         for metadata in bids_metas:
             if not metadata:
                 continue
+
+            # If the field is not available, fill that entry in the set with a False.
             echo_times.add(metadata.get('EchoTime', False))
             channel_names.add(metadata.get('CoilString', False))
             image_types.update(metadata.get('ImageType', [False]))
+
         is_multiecho = len(set(filter(bool, echo_times))) > 1  # Check for varying echo times
         is_uncombined = len(set(filter(bool, channel_names))) > 1  # Check for uncombined data
         is_complex = 'M' in image_types and 'P' in image_types  # Determine if data are complex (magnitude + phase)

heudiconv/tests/test_convert.py

@@ -1,11 +1,12 @@
 """Test functions in heudiconv.convert module.
 """
 import pytest
-
-from heudiconv.convert import (update_complex_name,
-                               update_multiecho_name,
-                               update_uncombined_name)
 from heudiconv.bids import BIDSError
+from heudiconv.convert import (
+    update_complex_name,
+    update_multiecho_name,
+    update_uncombined_name,
+)
 
 
 def test_update_complex_name():
@@ -19,16 +20,19 @@ def test_update_complex_name():
     out_fn_true = 'sub-X_ses-Y_task-Z_rec-phase_run-01_sbref'
     out_fn_test = update_complex_name(metadata, fn, suffix)
     assert out_fn_test == out_fn_true
+
     # Catch an unsupported type and *do not* update
     fn = 'sub-X_ses-Y_task-Z_run-01_phase'
     out_fn_test = update_complex_name(metadata, fn, suffix)
     assert out_fn_test == fn
+
     # Data type is missing from metadata so use suffix
     fn = 'sub-X_ses-Y_task-Z_run-01_sbref'
     metadata = {'ImageType': ['ORIGINAL', 'PRIMARY', 'MB', 'TE3', 'ND', 'MOSAIC']}
     out_fn_true = 'sub-X_ses-Y_task-Z_rec-3_run-01_sbref'
     out_fn_test = update_complex_name(metadata, fn, suffix)
     assert out_fn_test == out_fn_true
+
     # Catch existing field with value that *does not match* metadata
     # and raise Exception
     fn = 'sub-X_ses-Y_task-Z_rec-magnitude_run-01_sbref'
@@ -50,15 +54,37 @@ def test_update_multiecho_name():
     out_fn_true = 'sub-X_ses-Y_task-Z_run-01_echo-1_bold'
     out_fn_test = update_multiecho_name(metadata, fn, echo_times)
     assert out_fn_test == out_fn_true
+
     # EchoNumber field is missing from metadata, so use echo_times
     metadata = {'EchoTime': 0.01}
     out_fn_test = update_multiecho_name(metadata, fn, echo_times)
     assert out_fn_test == out_fn_true
+
     # Catch an unsupported type and *do not* update
     fn = 'sub-X_ses-Y_task-Z_run-01_phasediff'
     out_fn_test = update_multiecho_name(metadata, fn, echo_times)
     assert out_fn_test == fn
 
+    # EchoTime is missing, but use EchoNumber (which is the first thing it checks)
+    fn = 'sub-X_ses-Y_task-Z_run-01_bold'
+    out_fn_true = 'sub-X_ses-Y_task-Z_run-01_echo-1_bold'
+    metadata = {'EchoNumber': 1}
+    echo_times = [False, 0.02, 0.03]
+    out_fn_test = update_multiecho_name(metadata, fn, echo_times)
+    assert out_fn_test == out_fn_true
+
+    # Both EchoTime and EchoNumber are missing, which raises a KeyError
+    fn = 'sub-X_ses-Y_task-Z_run-01_bold'
+    metadata = {}
+    echo_times = [False, 0.02, 0.03]
+    with pytest.raises(KeyError):
+        update_multiecho_name(metadata, fn, echo_times)
+
+    # Providing echo times as something other than a list should raise a TypeError
+    fn = 'sub-X_ses-Y_task-Z_run-01_bold'
+    with pytest.raises(TypeError):
+        update_multiecho_name(metadata, fn, set(echo_times))
+
 
 def test_update_uncombined_name():
     """Unit testing for heudiconv.convert.update_uncombined_name(), which updates
@@ -71,8 +97,29 @@ def test_update_uncombined_name():
     out_fn_true = 'sub-X_ses-Y_task-Z_run-01_ch-01_bold'
     out_fn_test = update_uncombined_name(metadata, fn, channel_names)
     assert out_fn_test == out_fn_true
-    # CoilString field has no number in it
+
+    # CoilString field has no number in it, so we index the channel_names list
     metadata = {'CoilString': 'HEA;HEP'}
     out_fn_true = 'sub-X_ses-Y_task-Z_run-01_ch-04_bold'
     out_fn_test = update_uncombined_name(metadata, fn, channel_names)
     assert out_fn_test == out_fn_true
+
+    # Extract the number from the CoilString and use that
+    channel_names = ['H1', 'B1', 'H3', 'HEA;HEP']
+    metadata = {'CoilString': 'H1'}
+    out_fn_true = 'sub-X_ses-Y_task-Z_run-01_ch-01_bold'
+    out_fn_test = update_uncombined_name(metadata, fn, channel_names)
+    assert out_fn_test == out_fn_true
+
+    # NOTE: Extracting the number does not protect against multiple coils with the same number
+    # (but, say, different letters)
+    # Note that this is still "ch-01"
+    metadata = {'CoilString': 'B1'}
+    out_fn_true = 'sub-X_ses-Y_task-Z_run-01_ch-01_bold'
+    out_fn_test = update_uncombined_name(metadata, fn, channel_names)
+    assert out_fn_test == out_fn_true
+
+    # Providing echo times as something other than a list should raise a TypeError
+    fn = 'sub-X_ses-Y_task-Z_run-01_bold'
+    with pytest.raises(TypeError):
+        update_uncombined_name(metadata, fn, set(channel_names))