Merge pull request #159 from oesteban/enh/increase-coverage

ENH: Increase unit tests coverage of ``sdcflows.fieldmaps``

Author: oesteban

sdcflows/fieldmaps.py

@@ -50,8 +50,7 @@ def _type_setter(obj, attribute, value):
     """Make sure the type of estimation is not changed."""
     if obj.method == value:
         return value
-
-    if obj.method != EstimatorType.UNKNOWN and obj.method != value:
+    elif obj.method != EstimatorType.UNKNOWN:
         raise TypeError(f"Cannot change determined method {obj.method} to {value}.")
 
     if value not in (
@@ -99,6 +98,13 @@ class FieldmapFile:
     >>> f.metadata['TotalReadoutTime']
     0.005
 
+    >>> f = FieldmapFile(
+    ...     dsA_dir / "sub-01" / "fmap" / "sub-01_dir-LR_epi.nii.gz",
+    ...     metadata={"TotalReadoutTime": None},
+    ... )  # doctest: +IGNORE_EXCEPTION_DETAIL
+    Traceback (most recent call last):
+    MetadataError:
+
     >>> f = FieldmapFile(
     ...     dsA_dir / "sub-01" / "fmap" / "sub-01_dir-LR_epi.nii.gz",
     ...     metadata={'TotalReadoutTime': 0.006}
@@ -171,17 +177,12 @@ class FieldmapFile:
     @path.validator
     def check_path(self, attribute, value):
         """Validate a fieldmap path."""
-        if isinstance(value, BIDSFile):
-            value = Path(value.path)
-        elif isinstance(value, str):
-            value = Path(value)
-
         if not value.is_file():
             raise FileNotFoundError(
                 f"File path <{value}> does not exist, is a broken link, or it is not a file"
             )
 
-        if not str(value).endswith((".nii", ".nii.gz")):
+        if not value.name.endswith((".nii", ".nii.gz")):
             raise ValueError(f"File path <{value}> does not look like a NIfTI file.")
 
         suffix = re.search(r"(?<=_)\w+(?=\.nii)", value.name).group()
@@ -208,8 +209,11 @@ def __attrs_post_init__(self):
 
         # Attempt to infer a bids_root folder
         relative_path = relative_to_root(self.path)
-        if str(relative_path) != str(self.path):
-            self.bids_root = Path(str(self.path)[: -len(str(relative_path))])
+        self.bids_root = (
+            Path(str(self.path)[: -len(str(relative_path))])
+            if str(relative_path) != str(self.path)
+            else None
+        )
 
         # Check for REQUIRED metadata (depends on suffix.)
         if self.suffix in ("bold", "dwi", "epi", "sbref"):
@@ -222,10 +226,10 @@ def __attrs_post_init__(self):
 
             try:
                 get_trt(self.metadata, in_file=self.path)
-            except ValueError:
+            except ValueError as exc:
                 raise MetadataError(
                     f"Missing readout timing information for <{self.path}>."
-                )
+                ) from exc
 
         elif self.suffix == "fieldmap" and "Units" not in self.metadata:
             raise MetadataError(f"Missing 'Units' for <{self.path}>.")

sdcflows/tests/data/dsA/sub-01/anat/sub-01_FLAIR.nii.gz

@@ -1,11 +1,25 @@
 """test_fieldmaps."""
+from collections import namedtuple
+import shutil
 import pytest
+import bids
 from .. import fieldmaps as fm
 
 
 def test_FieldmapFile(dsA_dir):
     """Test one existing file."""
-    fm.FieldmapFile(dsA_dir / "sub-01" / "anat" / "sub-01_T1w.nii.gz")
+    f1 = fm.FieldmapFile(dsA_dir / "sub-01" / "anat" / "sub-01_T1w.nii.gz")
+    f2 = fm.FieldmapFile(str(dsA_dir / "sub-01" / "anat" / "sub-01_T1w.nii.gz"))
+    f3 = fm.FieldmapFile(
+        bids.layout.BIDSFile(str(dsA_dir / "sub-01" / "anat" / "sub-01_T1w.nii.gz"))
+    )
+    assert f1 == f2 == f3
+
+    with pytest.raises(ValueError):
+        fm.FieldmapFile(dsA_dir / "sub-01" / "fmap" / "sub-01_dir-AP_epi.json")
+
+    with pytest.raises(ValueError):
+        fm.FieldmapFile(dsA_dir / "sub-01" / "anat" / "sub-01_FLAIR.nii.gz")
 
 
 @pytest.mark.parametrize(
@@ -203,3 +217,78 @@ def test_FieldmapEstimationIdentifier(monkeypatch, dsA_dir):
         fm.get_identifier("file", by="invalid")
 
     fm.clear_registry()
+
+
+def test_type_setter():
+    """Cover the _type_setter routine."""
+    obj = namedtuple("FieldmapEstimation", ("method",))(method=fm.EstimatorType.UNKNOWN)
+    with pytest.raises(ValueError):
+        fm._type_setter(obj, "method", 10)
+
+    obj = namedtuple("FieldmapEstimation", ("method",))(method=fm.EstimatorType.PEPOLAR)
+    assert (
+        fm._type_setter(obj, "method", fm.EstimatorType.PEPOLAR)
+        == fm.EstimatorType.PEPOLAR
+    )
+
+
+def test_FieldmapEstimation_missing_files(tmpdir, dsA_dir):
+    """Exercise some FieldmapEstimation checks."""
+    tmpdir.chdir()
+    tmpdir.mkdir("data")
+
+    # fieldmap - no magnitude
+    path = dsA_dir / "sub-01" / "fmap" / "sub-01_fieldmap.nii.gz"
+    shutil.copy(path, f"data/{path.name}")
+
+    with pytest.raises(
+        ValueError,
+        match=r"A fieldmap or phase-difference .* \(magnitude file\) is missing.*",
+    ):
+        fm.FieldmapEstimation(
+            [fm.FieldmapFile("data/sub-01_fieldmap.nii.gz", metadata={"Units": "Hz"})]
+        )
+
+    # phase1/2 - no magnitude2
+    path = dsA_dir / "sub-01" / "fmap" / "sub-01_phase1.nii.gz"
+    shutil.copy(path, f"data/{path.name}")
+
+    path = dsA_dir / "sub-01" / "fmap" / "sub-01_magnitude1.nii.gz"
+    shutil.copy(path, f"data/{path.name}")
+
+    path = dsA_dir / "sub-01" / "fmap" / "sub-01_phase2.nii.gz"
+    shutil.copy(path, f"data/{path.name}")
+
+    with pytest.raises(
+        ValueError, match=r".* \(phase1/2\) .* \(magnitude2 file\) is missing.*"
+    ):
+        fm.FieldmapEstimation(
+            [
+                fm.FieldmapFile(
+                    "data/sub-01_phase1.nii.gz", metadata={"EchoTime": 0.004}
+                ),
+                fm.FieldmapFile(
+                    "data/sub-01_phase2.nii.gz", metadata={"EchoTime": 0.007}
+                ),
+            ]
+        )
+
+    # pepolar - only one PE
+    path = dsA_dir / "sub-01" / "fmap" / "sub-01_dir-AP_epi.nii.gz"
+    shutil.copy(path, f"data/{path.name}")
+
+    path = dsA_dir / "sub-01" / "dwi" / "sub-01_dir-AP_dwi.nii.gz"
+    shutil.copy(path, f"data/{path.name}")
+    with pytest.raises(ValueError, match="Only one phase-encoding direction <j>.*"):
+        fm.FieldmapEstimation(
+            [
+                fm.FieldmapFile(
+                    "data/sub-01_dir-AP_epi.nii.gz",
+                    metadata={"PhaseEncodingDirection": "j", "TotalReadoutTime": 0.004},
+                ),
+                fm.FieldmapFile(
+                    "data/sub-01_dir-AP_dwi.nii.gz",
+                    metadata={"PhaseEncodingDirection": "j", "TotalReadoutTime": 0.004},
+                ),
+            ]
+        )

sdcflows/tests/test_fieldmaps.py

@@ -161,7 +161,11 @@ def get_trt(in_meta, in_file=None):
 
     # Use case 1: TRT is defined
     if "TotalReadoutTime" in in_meta:
-        return in_meta.get("TotalReadoutTime")
+        trt = in_meta.get("TotalReadoutTime")
+        if not trt:
+            raise ValueError(f"'{trt}'")
+
+        return trt
 
     # npe = N voxels PE direction
     pe_index = "ijk".index(in_meta["PhaseEncodingDirection"][0])