From 9551112fecb5315b67e6738b104449ff858da506 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 17 Dec 2020 10:49:11 +0100 Subject: [PATCH] enh: increase unit tests coverage of ``sdcflows.fieldmaps`` Adds some minimal checks to the new module. --- sdcflows/fieldmaps.py | 28 +++--- .../data/dsA/sub-01/anat/sub-01_FLAIR.nii.gz | 0 sdcflows/tests/test_fieldmaps.py | 91 ++++++++++++++++++- sdcflows/utils/epimanip.py | 6 +- 4 files changed, 111 insertions(+), 14 deletions(-) create mode 100644 sdcflows/tests/data/dsA/sub-01/anat/sub-01_FLAIR.nii.gz diff --git a/sdcflows/fieldmaps.py b/sdcflows/fieldmaps.py index e62995eed6..ff42810364 100644 --- a/sdcflows/fieldmaps.py +++ b/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}>.") diff --git a/sdcflows/tests/data/dsA/sub-01/anat/sub-01_FLAIR.nii.gz b/sdcflows/tests/data/dsA/sub-01/anat/sub-01_FLAIR.nii.gz new file mode 100644 index 0000000000..e69de29bb2 diff --git a/sdcflows/tests/test_fieldmaps.py b/sdcflows/tests/test_fieldmaps.py index 820e0cb97c..450d5538cb 100644 --- a/sdcflows/tests/test_fieldmaps.py +++ b/sdcflows/tests/test_fieldmaps.py @@ -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 .*"): + 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}, + ), + ] + ) diff --git a/sdcflows/utils/epimanip.py b/sdcflows/utils/epimanip.py index dab5213939..79d56dba25 100644 --- a/sdcflows/utils/epimanip.py +++ b/sdcflows/utils/epimanip.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])