RF: embed_nifti -> embed_dicom_and_nifti_metadata and not invoke it if min_meta

heudiconv/convert.py

@@ -325,18 +325,17 @@ def convert(items, converter, scaninfo_suffix, custom_callable, with_prov,
                     )
 
         # add the taskname field to the json file(s):
-        add_taskname_to_infofile( bids_outfiles )
+        add_taskname_to_infofile(bids_outfiles)
 
         if len(bids_outfiles) > 1:
             lgr.warning("For now not embedding BIDS and info generated "
                         ".nii.gz itself since sequence produced "
                         "multiple files")
         elif not bids_outfiles:
             lgr.debug("No BIDS files were produced, nothing to embed to then")
-        elif outname:
+        elif outname and not min_meta:
             embed_metadata_from_dicoms(bids_options, item_dicoms, outname, outname_bids,
-                                       prov_file, scaninfo, tempdirs, with_prov,
-                                       min_meta)
+                                       prov_file, scaninfo, tempdirs, with_prov)
         if scaninfo and op.exists(scaninfo):
             lgr.info("Post-treating %s file", scaninfo)
             treat_infofile(scaninfo)

heudiconv/dicoms.py

@@ -6,7 +6,13 @@
 import tarfile
 
 from .external.pydicom import dcm
-from .utils import load_json, get_typed_attr, set_readonly, SeqInfo
+from .utils import (
+    get_typed_attr,
+    load_json,
+    save_json,
+    SeqInfo,
+    set_readonly,
+)
 
 import warnings
 with warnings.catch_warnings():
@@ -386,14 +392,10 @@ def _assign_dicom_time(ti):
     return outtar
 
 
-def embed_nifti(dcmfiles, niftifile, infofile, bids_info, min_meta):
-    """
-
-    If `niftifile` doesn't exist, it gets created out of the `dcmfiles` stack,
-    and json representation of its meta_ext is returned (bug since should return
-    both niftifile and infofile?)
+def embed_dicom_and_nifti_metadata(dcmfiles, niftifile, infofile, bids_info):
+    """Embed metadata from nifti (affine etc) and dicoms into infofile (json)
 
-    if `niftifile` exists, its affine's orientation information is used while
+    `niftifile` should exist. Its affine's orientation information is used while
     establishing new `NiftiImage` out of dicom stack and together with `bids_info`
     (if provided) is dumped into json `infofile`
 
@@ -402,68 +404,52 @@ def embed_nifti(dcmfiles, niftifile, infofile, bids_info, min_meta):
     dcmfiles
     niftifile
     infofile
-    bids_info
-    min_meta
-
-    Returns
-    -------
-    niftifile, infofile
+    bids_info: dict
+      Additional metadata to be embedded. `infofile` is overwritten if exists,
+      so here you could pass some metadata which would overload (at the first
+      level of the dict structure, no recursive fancy updates) what is obtained
+      from nifti and dicoms
 
     """
     # imports for nipype
     import nibabel as nb
     import os.path as op
     import json
     import re
+    from heudiconv.utils import save_json
+
+    from heudiconv.external.dcmstack import ds
+    stack = ds.parse_and_stack(dcmfiles, force=True).values()
+    if len(stack) > 1:
+        raise ValueError('Found multiple series')
+    # may be odict now - iter to be safe
+    stack = next(iter(stack))
+
+    if not op.exists(niftifile):
+        raise NotImplementedError(
+            "%s does not exist. "
+            "We are not producing new nifti files here any longer. "
+            "Use dcm2niix directly or .convert.nipype_convert helper ."
+            % niftifile
+        )
 
-    if not min_meta:
-        from heudiconv.external.dcmstack import ds
-        stack = ds.parse_and_stack(dcmfiles, force=True).values()
-        if len(stack) > 1:
-            raise ValueError('Found multiple series')
-        # may be odict now - iter to be safe
-        stack = next(iter(stack))
-
-        # Create the nifti image using the data array
-        if not op.exists(niftifile):
-            nifti_image = stack.to_nifti(embed_meta=True)
-            nifti_image.to_filename(niftifile)
-            return ds.NiftiWrapper(nifti_image).meta_ext.to_json()
-
-        orig_nii = nb.load(niftifile)
-        aff = orig_nii.affine
-        ornt = nb.orientations.io_orientation(aff)
-        axcodes = nb.orientations.ornt2axcodes(ornt)
-        new_nii = stack.to_nifti(voxel_order=''.join(axcodes), embed_meta=True)
-        meta = ds.NiftiWrapper(new_nii).meta_ext.to_json()
-
-    meta_info = None if min_meta else json.loads(meta)
+    orig_nii = nb.load(niftifile)
+    aff = orig_nii.affine
+    ornt = nb.orientations.io_orientation(aff)
+    axcodes = nb.orientations.ornt2axcodes(ornt)
+    new_nii = stack.to_nifti(voxel_order=''.join(axcodes), embed_meta=True)
+    meta_info = ds.NiftiWrapper(new_nii).meta_ext.to_json()
+    meta_info = json.loads(meta_info)
 
     if bids_info:
+        meta_info.update(bids_info)
 
-        if min_meta:
-            meta_info = bids_info
-        else:
-            # make nice with python 3 - same behavior?
-            meta_info = meta_info.copy()
-            meta_info.update(bids_info)
-            # meta_info = dict(meta_info.items() + bids_info.items())
-        try:
-            meta_info['TaskName'] = re.search(
-                r'(?<=_task-)\w+', op.basename(infofile)
-            ).group(0).split('_')[0]
-        except AttributeError:
-            pass
     # write to outfile
-    with open(infofile, 'wt') as fp:
-        json.dump(meta_info, fp, indent=3, sort_keys=True)
-
-    return niftifile, infofile
+    save_json(infofile, meta_info)
 
 
 def embed_metadata_from_dicoms(bids_options, item_dicoms, outname, outname_bids,
-                               prov_file, scaninfo, tempdirs, with_prov,
-                               min_meta):
+                               prov_file, scaninfo, tempdirs, with_prov):
     """
     Enhance sidecar information file with more information from DICOMs
 
@@ -477,7 +463,6 @@ def embed_metadata_from_dicoms(bids_options, item_dicoms, outname, outname_bids,
     scaninfo
     tempdirs
     with_prov
-    min_meta
 
     Returns
     -------
@@ -490,14 +475,13 @@ def embed_metadata_from_dicoms(bids_options, item_dicoms, outname, outname_bids,
     item_dicoms = list(map(op.abspath, item_dicoms))
 
     embedfunc = Node(Function(input_names=['dcmfiles', 'niftifile', 'infofile',
-                                           'bids_info', 'min_meta'],
+                                           'bids_info',],
                               output_names=['outfile', 'meta'],
-                              function=embed_nifti),
+                              function=embed_dicom_and_nifti_metadata),
                      name='embedder')
     embedfunc.inputs.dcmfiles = item_dicoms
     embedfunc.inputs.niftifile = op.abspath(outname)
     embedfunc.inputs.infofile = op.abspath(scaninfo)
-    embedfunc.inputs.min_meta = min_meta
     embedfunc.inputs.bids_info = load_json(op.abspath(outname_bids)) if (bids_options is not None) else None
     embedfunc.base_dir = tmpdir
     cwd = os.getcwd()

heudiconv/tests/test_dicoms.py

@@ -5,8 +5,12 @@
 
 from heudiconv.external.pydicom import dcm
 from heudiconv.cli.run import main as runner
-from heudiconv.dicoms import parse_private_csa_header, embed_nifti
-from .utils import TESTS_DATA_PATH
+from heudiconv.convert import nipype_convert
+from heudiconv.dicoms import parse_private_csa_header, embed_dicom_and_nifti_metadata
+from .utils import (
+    assert_cwd_unchanged,
+    TESTS_DATA_PATH,
+)
 
 # Public: Private DICOM tags
 DICOM_FIELDS_TO_TEST = {
@@ -26,35 +30,37 @@ def test_private_csa_header(tmpdir):
         runner(['--files', dcm_file, '-c' 'none', '-f', 'reproin'])
 
 
-def test_nifti_embed(tmpdir):
+@assert_cwd_unchanged(ok_to_chdir=True)  # so we cd back after tmpdir.chdir
+def test_embed_dicom_and_nifti_metadata(tmpdir):
     """Test dcmstack's additional fields"""
     tmpdir.chdir()
     # set up testing files
     dcmfiles = [op.join(TESTS_DATA_PATH, 'axasc35.dcm')]
     infofile = 'infofile.json'
 
-    # 1) nifti does not exist
-    out = embed_nifti(dcmfiles, 'nifti.nii', 'infofile.json', None, False)
-    # string -> json
-    out = json.loads(out)
-    # should have created nifti file
-    assert op.exists('nifti.nii')
+    out_prefix = str(tmpdir / "nifti")
+    # 1) nifti does not exist -- no longer supported
+    with pytest.raises(NotImplementedError):
+        embed_dicom_and_nifti_metadata(dcmfiles, out_prefix + '.nii.gz', infofile, None)
 
+    # we should produce nifti using our "standard" ways
+    nipype_out, prov_file = nipype_convert(
+        dcmfiles, prefix=out_prefix, with_prov=False,
+        bids_options=None, tmpdir=str(tmpdir))
+    niftifile = nipype_out.outputs.converted_files
+
+    assert op.exists(niftifile)
     # 2) nifti exists
-    nifti, info = embed_nifti(dcmfiles, 'nifti.nii', 'infofile.json', None, False)
-    assert op.exists(nifti)
-    assert op.exists(info)
-    with open(info) as fp:
+    embed_dicom_and_nifti_metadata(dcmfiles, niftifile, infofile, None)
+    assert op.exists(infofile)
+    with open(infofile) as fp:
         out2 = json.load(fp)
 
-    assert out == out2
-
     # 3) with existing metadata
     bids = {"existing": "data"}
-    nifti, info = embed_nifti(dcmfiles, 'nifti.nii', 'infofile.json', bids, False)
-    with open(info) as fp:
+    embed_dicom_and_nifti_metadata(dcmfiles, niftifile, infofile, bids)
+    with open(infofile) as fp:
         out3 = json.load(fp)
 
-    assert out3["existing"]
-    del out3["existing"]
-    assert out3 == out2 == out
+    assert out3.pop("existing") == "data"
+    assert out3 == out2

heudiconv/tests/utils.py

@@ -1,9 +1,17 @@
+from functools import wraps
+import os
 import os.path as op
+import sys
+
 import heudiconv.heuristics
 
+
 HEURISTICS_PATH = op.join(heudiconv.heuristics.__path__[0])
 TESTS_DATA_PATH = op.join(op.dirname(__file__), 'data')
 
+import logging
+lgr = logging.getLogger(__name__)
+
 
 def gen_heudiconv_args(datadir, outdir, subject, heuristic_file,
                        anon_cmd=None, template=None, xargs=None):
@@ -58,3 +66,51 @@ def fetch_data(tmpdir, dataset, getpath=None):
     getdir = targetdir + (op.sep + getpath if getpath is not None else '')
     ds.get(getdir)
     return targetdir
+
+
+def assert_cwd_unchanged(ok_to_chdir=False):
+    """Decorator to test whether the current working directory remains unchanged
+
+    Provenance: based on the one in datalad, but simplified.
+
+    Parameters
+    ----------
+    ok_to_chdir: bool, optional
+      If True, allow to chdir, so this decorator would not then raise exception
+      if chdir'ed but only return to original directory
+    """
+
+    def decorator(func=None):  # =None to avoid pytest treating it as a fixture
+        @wraps(func)
+        def newfunc(*args, **kwargs):
+            cwd_before = os.getcwd()
+            exc = None
+            try:
+                return func(*args, **kwargs)
+            except Exception as exc_:
+                exc = exc_
+            finally:
+                try:
+                    cwd_after = os.getcwd()
+                except OSError as e:
+                    lgr.warning("Failed to getcwd: %s" % e)
+                    cwd_after = None
+
+                if cwd_after != cwd_before:
+                    os.chdir(cwd_before)
+                    if not ok_to_chdir:
+                        lgr.warning(
+                            "%s changed cwd to %s. Mitigating and changing back to %s"
+                            % (func, cwd_after, cwd_before))
+                        # If there was already exception raised, we better reraise
+                        # that one since it must be more important, so not masking it
+                        # here with our assertion
+                        if exc is None:
+                            assert cwd_before == cwd_after, \
+                                     "CWD changed from %s to %s" % (cwd_before, cwd_after)
+
+                if exc is not None:
+                    raise exc
+        return newfunc
+
+    return decorator