Merge pull request #129 from dartmouth-pbs/merge-enh-dbic2

Fixes from DBIC field-test site and code rodeo coding session (AKA reaching convergence)

Author: yarikoptic

.travis.yml

@@ -36,7 +36,7 @@ install:
   - git config --global user.name "Travis Almighty"
 
 script:
-  - coverage run `which py.test` -s -v tests heuristics
+  - coverage run `which py.test` -s -v tests heuristics/*.py
 
 after_success:
   - codecov

heudiconv/__init__.py

@@ -1,5 +1,12 @@
 # set logger handler
 import logging
-logging.getLogger(__name__).addHandler(logging.NullHandler())
-
+import os
 from .info import (__version__, __packagename__)
+
+# Rudimentary logging support.
+lgr = logging.getLogger(__name__)
+logging.basicConfig(
+    format='%(levelname)s: %(message)s',
+    level=getattr(logging, os.environ.get('HEUDICONV_LOG_LEVEL', 'INFO'))
+)
+lgr.debug("Starting the abomination")  # just to "run-test" logging

heudiconv/bids.py

@@ -61,10 +61,10 @@ def populate_bids_templates(path, defaults={}):
     create_file_if_missing(op.join(path, 'CHANGES'),
         "0.0.1  Initial data acquired\n"
         "TODOs:\n\t- verify and possibly extend information in participants.tsv"
-        "(see for example http://datasets.datalad.org/?dir=/openfmri/ds000208)"
-        "\n\t- fill out dataset_description.json, README, sourcedata/README "
-        "(if present)\n\t- provide _events.tsv file for each _bold.nii.gz with "
-        "onsets of events (see  '8.5 Task events'  of BIDS specification)")
+        " (see for example http://datasets.datalad.org/?dir=/openfmri/ds000208)"
+        "\n\t- fill out dataset_description.json, README, sourcedata/README"
+        " (if present)\n\t- provide _events.tsv file for each _bold.nii.gz with"
+        " onsets of events (see  '8.5 Task events'  of BIDS specification)")
     create_file_if_missing(op.join(path, 'README'),
         "TODO: Provide description for the dataset -- basic details about the "
         "study, possibly pointing to pre-registration (if public or embargoed)")
@@ -92,7 +92,7 @@ def populate_bids_templates(path, defaults={}):
         events_file = fpath[:-len(suf)] + '_events.tsv'
         lgr.debug("Generating %s", events_file)
         with open(events_file, 'w') as f:
-            f.write("onset\tduration\ttrial_type\tresponse_time\tTODO -- fill in rows and add more tab-separated columns if desired")
+            f.write("onset\tduration\ttrial_type\tresponse_time\tstim_file\tTODO -- fill in rows and add more tab-separated columns if desired")
     # extract tasks files stubs
     for task_acq, fields in tasks.items():
         task_file = op.join(path, task_acq + '_bold.json')

heudiconv/cli/run.py

@@ -8,6 +8,7 @@
 from ..utils import load_heuristic, anonymize_sid, treat_infofile, SeqInfo
 from ..convert import prep_conversion
 
+from ..bids import (populate_bids_templates, tuneup_bids_json_files)
 import inspect
 import logging
 lgr = logging.getLogger(__name__)
@@ -87,11 +88,20 @@ def process_extra_commands(outdir, args):
 def main(argv=None):
     parser = get_parser()
     args = parser.parse_args(argv)
+    # To be done asap so anything random is deterministic
+    if args.random_seed is not None:
+        import random
+        random.seed(args.random_seed)
+        import numpy
+        numpy.random.seed(args.random_seed)
     if args.debug:
         lgr.setLevel(logging.DEBUG)
-
-    if args.files and args.subjs:
-        raise ValueError("Unable to processes `--subjects` with files")
+    # Should be possible but only with a single subject -- will be used to
+    # override subject deduced from the DICOMs
+    if args.files and args.subjs and len(args.subjs) > 1:
+        raise ValueError(
+            "Unable to processes multiple `--subjects` with files"
+        )
 
     if args.debug:
         setup_exceptionhook()
@@ -119,9 +129,10 @@ def get_parser():
                         help='list of subjects - required for dicom template. '
                         'If not provided, DICOMS would first be "sorted" and '
                         'subject IDs deduced by the heuristic')
-    parser.add_argument('-c', '--converter', default='dcm2niix',
+    parser.add_argument('-c', '--converter',
+                        default='dcm2niix',
                         choices=('dcm2niix', 'none'),
-                        help='tool to use for dicom conversion. Setting to '
+                        help='tool to use for DICOM conversion. Setting to '
                         '"none" disables the actual conversion step -- useful'
                         'for testing heuristics.')
     parser.add_argument('-o', '--outdir', default=os.getcwd(),
@@ -149,7 +160,7 @@ def get_parser():
     parser.add_argument('-b', '--bids', action='store_true',
                         help='flag for output into BIDS structure')
     parser.add_argument('--overwrite', action='store_true', default=False,
-                        help='flag to allow overwrite existing files')
+                        help='flag to allow overwriting existing converted files')
     parser.add_argument('--datalad', action='store_true',
                         help='Store the entire collection as DataLad '
                         'dataset(s). Small files will be committed directly to '
@@ -171,7 +182,8 @@ def get_parser():
     parser.add_argument('--minmeta', action='store_true',
                         help='Exclude dcmstack meta information in sidecar '
                         'jsons')
-
+    parser.add_argument('--random-seed', type=int, default=None,
+                        help='Random seed to initialize RNG')
     submission = parser.add_argument_group('Conversion submission options')
     submission.add_argument('-q', '--queue', default=None,
                             help='select batch system to submit jobs to instead'
@@ -215,6 +227,9 @@ def process_args(args):
 
     for (locator, session, sid), files_or_seqinfo in study_sessions.items():
 
+        # Allow for session to be overloaded from command line
+        if args.session is not None:
+            session = args.session
         if not len(files_or_seqinfo):
             raise ValueError("nothing to process?")
         # that is how life is ATM :-/ since we don't do sorting if subj
@@ -250,7 +265,7 @@ def process_args(args):
                              sid,
                              args.anon_cmd,
                              args.converter,
-                             args.session,
+                             session,
                              args.with_prov,
                              args.bids)
             continue
@@ -269,10 +284,10 @@ def process_args(args):
             from ..external.dlad import prepare_datalad
             dlad_sid = sid if not anon_sid else anon_sid
             dl_msg = prepare_datalad(anon_study_outdir, anon_outdir, dlad_sid,
-                                     args.session, seqinfo, dicoms, args.bids)
+                                     session, seqinfo, dicoms, args.bids)
 
         lgr.info("PROCESSING STARTS: {0}".format(
-            str(dict(subject=sid, outdir=study_outdir, session=args.session))))
+            str(dict(subject=sid, outdir=study_outdir, session=session))))
 
         prep_conversion(sid,
                         dicoms,
@@ -282,7 +297,7 @@ def process_args(args):
                         anon_sid=anon_sid,
                         anon_outdir=anon_study_outdir,
                         with_prov=args.with_prov,
-                        ses=args.session,
+                        ses=session,
                         bids=args.bids,
                         seqinfo=seqinfo,
                         min_meta=args.minmeta,

heudiconv/convert.py

@@ -14,6 +14,8 @@
     set_readonly,
     clear_temp_dicoms,
     seqinfo_fields,
+    assure_no_file_exists,
+    file_md5sum
 )
 from .bids import (
     convert_sid_bids,
@@ -100,15 +102,35 @@ def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
     if not op.exists(idir):
         os.makedirs(idir)
 
-    shutil.copy(heuristic.filename, idir)
     ses_suffix = "_ses-%s" % ses if ses is not None else ""
     info_file = op.join(idir, '%s%s.auto.txt' % (sid, ses_suffix))
     edit_file = op.join(idir, '%s%s.edit.txt' % (sid, ses_suffix))
     filegroup_file = op.join(idir, 'filegroup%s.json' % ses_suffix)
 
+    # if conversion table(s) do not exist -- we need to prepare them
+    # (the *prepare* stage in https://github.com/nipy/heudiconv/issues/134)
+    reuse_conversion_table = op.exists(edit_file)
+    # We also might need to redo it if changes in the heuristic file
+    # detected
+    # ref: https://github.com/nipy/heudiconv/issues/84#issuecomment-330048609
+    # for more automagical wishes
+    target_heuristic_filename = op.join(idir, op.basename(heuristic.filename))
+    # TODO:
+    #  1. add a test
+    #  2. possibly extract into a dedicated function for easier logic flow here
+    #     and a dedicated unittest
+    if not reuse_conversion_table and \
+        op.exists(target_heuristic_filename) and \
+        file_md5sum(target_heuristic_filename) != file_md5sum(heuristic.filename):
+        reuse_conversion_table = False
+        lgr.info(
+            "Will not reuse existing conversion table files because heuristic "
+            "has changed"
+        )
+
     # MG - maybe add an option to force rerun?
     # related issue : https://github.com/nipy/heudiconv/issues/84
-    if op.exists(edit_file) and overwrite:
+    if reuse_conversion_table:
         lgr.info("Reloading existing filegroup.json "
                  "because %s exists", edit_file)
         info = read_config(edit_file)
@@ -122,6 +144,8 @@ def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
     else:
         # TODO -- might have been done outside already!
         # MG -- will have to try with both dicom template, files
+        assure_no_file_exists(target_heuristic_filename)
+        safe_copyfile(heuristic.filename, idir)
         if dicoms:
             seqinfo = group_dicoms_into_seqinfos(
                 dicoms,
@@ -131,6 +155,8 @@ def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
         seqinfo_list = list(seqinfo.keys())
         filegroup = {si.series_id: x for si, x in seqinfo.items()}
         dicominfo_file = op.join(idir, 'dicominfo%s.tsv' % ses_suffix)
+        # allow to overwrite even if was present under git-annex already
+        assure_no_file_exists(dicominfo_file)
         with open(dicominfo_file, 'wt') as fp:
             fp.write('\t'.join([val for val in seqinfo_fields]) + '\n')
             for seq in seqinfo_list:
@@ -139,7 +165,9 @@ def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
         info = heuristic.infotodict(seqinfo_list)
         lgr.debug("Writing to {}, {}, {}".format(info_file, edit_file,
                                                  filegroup_file))
+        assure_no_file_exists(info_file)
         write_config(info_file, info)
+        assure_no_file_exists(edit_file)
         write_config(edit_file, info)
         save_json(filegroup_file, filegroup)
 
@@ -150,7 +178,7 @@ def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
     else:
         tdir = op.join(anon_outdir, anon_sid)
 
-    if converter != 'none':
+    if converter.lower() != 'none':
         lgr.info("Doing conversion using %s", converter)
         cinfo = conversion_info(anon_sid, tdir, info, filegroup, ses)
         convert(cinfo,
@@ -218,8 +246,8 @@ def convert(items, converter, scaninfo_suffix, custom_callable, with_prov,
             os.makedirs(prefix_dirname)
 
         for outtype in outtypes:
-            lgr.debug("Processing %d dicoms for output type %s",
-                     len(item_dicoms), outtype)
+            lgr.debug("Processing %d dicoms for output type %s. Overwrite=%s",
+                     len(item_dicoms), outtype, overwrite)
             lgr.debug("Includes the following dicoms: %s", item_dicoms)
 
             seqtype = op.basename(op.dirname(prefix)) if bids else None
@@ -243,7 +271,8 @@ def convert(items, converter, scaninfo_suffix, custom_callable, with_prov,
 
                     bids_outfiles = save_converted_files(res, item_dicoms, bids,
                                                          outtype, prefix,
-                                                         outname_bids)
+                                                         outname_bids,
+                                                         overwrite=overwrite)
 
                     # save acquisition time information if it's BIDS
                     # at this point we still have acquisition date
@@ -257,15 +286,23 @@ def convert(items, converter, scaninfo_suffix, custom_callable, with_prov,
                         safe_copyfile(op.join(convertnode.base_dir,
                                               convertnode.name,
                                              'provenance.ttl'),
-                                      prov_file)
+                                      prov_file,
+                                      overwrite=overwrite)
                         prov_files.append(prov_file)
 
                     tempdirs.rmtree(tmpdir)
+                else:
+                    raise RuntimeError(
+                        "was asked to convert into %s but destination already exists"
+                        % (outname)
+                    )
 
         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")
         else:
             embed_metadata_from_dicoms(bids, item_dicoms, outname, outname_bids,
                                        prov_file, scaninfo, tempdirs, with_prov,
@@ -350,7 +387,7 @@ def nipype_convert(item_dicoms, prefix, with_prov, bids, tmpdir):
     return convertnode.run()
 
 
-def save_converted_files(res, item_dicoms, bids, outtype, prefix, outname_bids):
+def save_converted_files(res, item_dicoms, bids, outtype, prefix, outname_bids, overwrite):
     """Copy converted files from tempdir to output directory.
     Will rename files if necessary.
 
@@ -381,8 +418,8 @@ def save_converted_files(res, item_dicoms, bids, outtype, prefix, outname_bids):
 
     if isdefined(res.outputs.bvecs) and isdefined(res.outputs.bvals):
         outname_bvecs, outname_bvals = prefix + '.bvec', prefix + '.bval'
-        safe_copyfile(res.outputs.bvecs, outname_bvecs)
-        safe_copyfile(res.outputs.bvals, outname_bvals)
+        safe_copyfile(res.outputs.bvecs, outname_bvecs, overwrite)
+        safe_copyfile(res.outputs.bvals, outname_bvals, overwrite)
 
     if isinstance(res_files, list):
         # we should provide specific handling for fmap,
@@ -406,18 +443,18 @@ def save_converted_files(res, item_dicoms, bids, outtype, prefix, outname_bids):
 
         for fl, suffix, bids_file in zip(res_files, suffixes, bids_files):
             outname = "%s%s.%s" % (prefix, suffix, outtype)
-            safe_copyfile(fl, outname)
+            safe_copyfile(fl, outname, overwrite)
             if bids_file:
                 outname_bids_file = "%s%s.json" % (prefix, suffix)
-                safe_copyfile(bids_file, outname_bids_file)
+                safe_copyfile(bids_file, outname_bids_file, overwrite)
                 bids_outfiles.append(outname_bids_file)
     # res_files is not a list
     else:
         outname = "{}.{}".format(prefix, outtype)
-        safe_copyfile(res_files, outname)
+        safe_copyfile(res_files, outname, overwrite)
         if isdefined(res.outputs.bids):
             try:
-                safe_copyfile(res.outputs.bids, outname_bids)
+                safe_copyfile(res.outputs.bids, outname_bids, overwrite)
                 bids_outfiles.append(outname_bids)
             except TypeError as exc:  ##catch lists
                 raise TypeError("Multiple BIDS sidecars detected.")

heudiconv/dicoms.py

@@ -185,8 +185,7 @@ def group_dicoms_into_seqinfos(files, file_filter, dcmfilter, grouping):
         except AttributeError:
             series_desc = ''
 
-        motion_corrected = ('moco' in dcminfo.SeriesDescription.lower()
-                           or 'MOCO' in image_type)
+        motion_corrected = 'MOCO' in image_type
 
         if dcminfo.get([0x18,0x24], None):
             # GE and Philips scanners
@@ -448,6 +447,9 @@ def embed_metadata_from_dicoms(bids, item_dicoms, outname, outname_bids,
     from nipype import Node, Function
     tmpdir = tempdirs(prefix='embedmeta')
 
+    # We need to assure that paths are absolute if they are relative
+    item_dicoms = list(map(op.abspath, item_dicoms))
+
     embedfunc = Node(Function(input_names=['dcmfiles', 'niftifile', 'infofile',
                                            'bids_info', 'force', 'min_meta'],
                               output_names=['outfile', 'meta'],
@@ -464,6 +466,8 @@ def embed_metadata_from_dicoms(bids, item_dicoms, outname, outname_bids,
     embedfunc.inputs.force = True
     embedfunc.base_dir = tmpdir
     cwd = os.getcwd()
+    lgr.debug("Embedding into %s based on dicoms[0]=%s for nifti %s",
+              scaninfo, item_dicoms[0], outname)
     try:
         if op.lexists(scaninfo):
             # TODO: handle annexed file case

heudiconv/external/dlad.py

@@ -51,7 +51,7 @@ def add_to_datalad(topdir, studydir, msg, bids):
             ds_ = create(curdir_, dataset=superds,
                          force=True,
                          no_annex=True,
-                         shared_access='all',
+                         # shared_access='all',
                          annex_version=6)
             assert ds == ds_
         assert ds.is_installed()
@@ -94,8 +94,9 @@ def add_to_datalad(topdir, studydir, msg, bids):
                             "yet provided", ds)
             else:
                 dsh = ds.create(path='.heudiconv',
-                                force=True,
-                                shared_access='all')
+                                force=True
+                                # shared_access='all'
+                                )
         # Since .heudiconv could contain sensitive information
         # we place all files under annex and then add
         if create_file_if_missing(op.join(dsh_path, '.gitattributes'),

heudiconv/info.py

@@ -27,4 +27,5 @@
     'datalad': ['datalad']
 }
 
-EXTRA_REQUIRES['all'] = list(EXTRA_REQUIRES.values())
+# Flatten the lists
+EXTRA_REQUIRES['all'] = sum(EXTRA_REQUIRES.values(), [])