diff --git a/.travis.yml b/.travis.yml index 6c48f0b3..42f38840 100644 --- a/.travis.yml +++ b/.travis.yml @@ -27,7 +27,7 @@ env: - COVERAGE=coverage - DATALAD_DATASETS_TOPURL=http://datasets-tests.datalad.org matrix: - - DATALAD_REPO_DIRECT=yes + - DATALAD_REPO_VERSION=6 - DATALAD_REPO_VERSION=5 diff --git a/datalad_hirni/commands/dicom2spec.py b/datalad_hirni/commands/dicom2spec.py index 41a3df28..570b8e6d 100644 --- a/datalad_hirni/commands/dicom2spec.py +++ b/datalad_hirni/commands/dicom2spec.py @@ -42,15 +42,22 @@ def add_to_spec(ds_metadata, spec_list, basepath, # Note: The first 4 entries aren't a dict and have no # "approved flag", since they are automatically managed 'type': 'dicomseries', - #'status': None, # TODO: process state convention; flags 'location': op.relpath(ds_metadata['path'], basepath), 'uid': series['SeriesInstanceUID'], - 'dataset_id': ds_metadata['dsid'], - 'dataset_refcommit': ds_metadata['refcommit'], - 'converter': { - 'value': 'heudiconv' if series_is_valid(series) else 'ignore', - # TODO: not clear yet, what exactly to specify here - 'approved': False}, + 'dataset-id': ds_metadata['dsid'], + 'dataset-refcommit': ds_metadata['refcommit'], + 'converter': [{ + # special value 'hirni-dicom-converter' is interpreted by + # spec2bids and doesn't need a 'procedure-call' entry: + 'procedure-name': {'value': 'hirni-dicom-converter' + if series_is_valid(series) + else 'ignore', + 'approved': False}, + 'procedure-call': {'value': None, + 'approved': False}, + 'once-per-acquisition': {'value': True if series_is_valid(series) else None, + 'approved': False} + }] }) # get rules to apply: @@ -250,8 +257,8 @@ def __call__(path=None, spec=None, dataset=None, subject=None, for i in range(len(spec_series_list)): if spec_series_list[i]["type"] == "dicomseries" and \ heuristic.has_specval(spec_series_list[i], "converter") and \ - heuristic.get_specval(spec_series_list[i], "bids_run") in \ - [heuristic.get_specval(s, "bids_run") + heuristic.get_specval(spec_series_list[i], "bids-run") in \ + [heuristic.get_specval(s, "bids-run") for s in spec_series_list[i + 1:] if heuristic.get_specval(s, "description") == heuristic.get_specval( diff --git a/datalad_hirni/commands/spec2bids.py b/datalad_hirni/commands/spec2bids.py index 3bbef53c..1bc974aa 100644 --- a/datalad_hirni/commands/spec2bids.py +++ b/datalad_hirni/commands/spec2bids.py @@ -1,4 +1,4 @@ -"Convert DICOM data to BIDS based on the respective study specification" +"""Convert DICOM data to BIDS based on the respective study specification""" __docformat__ = 'restructuredtext' @@ -87,12 +87,15 @@ def __call__(specfile, dataset=None, anonymize=False): ) if op.isdir(spec_path): - if op.realpath(op.join(spec_path, op.pardir)) == op.realpath(dataset.path): + if op.realpath(op.join(spec_path, op.pardir)) == \ + op.realpath(dataset.path): spec_path = op.join( spec_path, - dataset.config.get("datalad.hirni.studyspec.filename", - "studyspec.json") + dataset.config.get( + "datalad.hirni.studyspec.filename", + "studyspec.json") ) + # TODO: check existence of that file! else: yield get_status_dict( action='spec2bids', @@ -102,7 +105,7 @@ def __call__(specfile, dataset=None, anonymize=False): "acquisition directory".format(spec_path) ) - ran_heudiconv = False + ran_procedure = dict() # relative path to spec to be recorded: rel_spec_path = relpath(spec_path, dataset.path) \ @@ -112,6 +115,33 @@ def __call__(specfile, dataset=None, anonymize=False): # wrt conversion: for spec_snippet in load_stream(spec_path): + # TODO: value 'ignore'!? + + if 'converter' not in spec_snippet: + # no conversion procedures defined at all: + yield get_status_dict( + action='spec2bids', + path=spec_path, + snippet=spec_snippet, + status='notneeded', + ) + continue + + procedure_list = spec_snippet['converter'] + if not procedure_list: + # no conversion procedures defined at all: + yield get_status_dict( + action='spec2bids', + path=spec_path, + snippet=spec_snippet, + status='notneeded', + ) + continue + + # accept a single dict as a one item list: + if isinstance(procedure_list, dict): + procedure_list = [procedure_list] + # build a dict available for placeholders in format strings: # Note: This is flattening the structure since we don't need # value/approved for the substitutions. In addition 'subject' @@ -119,93 +149,104 @@ def __call__(specfile, dataset=None, anonymize=False): # 'bids_subject' instead the value of which depends on the # --anonymize switch. # Additionally 'location' is recomputed to be relative to - # dataset.path, since this is where the converters are running + # dataset.path, since this is where the procedures are running # from within. replacements = dict() for k, v in spec_snippet.items(): if k == 'subject': + if not anonymize: + replacements['bids-subject'] = v['value'] + elif k == 'anon-subject': if anonymize: - continue - else: - replacements['bids_subject'] = v['value'] - elif k == 'anon_subject': - if anonymize: - replacements['bids_subject'] = v['value'] - else: - continue + replacements['bids-subject'] = v['value'] elif k == 'location': replacements[k] = op.join(op.dirname(rel_spec_path), v) - elif k == 'converter_path': - replacements[k] = op.join(op.dirname(rel_spec_path), v['value']) + elif k == 'converter': + # 'converter' is a list of dicts (not suitable for + # substitutions) and it makes little sense to be + # referenced by converter format strings anyway: + continue else: replacements[k] = v['value'] if isinstance(v, dict) else v - dataset.config.overrides = { - "datalad.run.substitutions._hs": replacements} - dataset.config.reload() - - if not ran_heudiconv and \ - heuristic.has_specval(spec_snippet, 'converter') and \ - heuristic.get_specval(spec_snippet, 'converter') == 'heudiconv': - # TODO: location! + # build dict to patch os.environ with for passing + # replacements on to procedures: + env_subs = dict() + for k, v in replacements.items(): + env_subs['DATALAD_RUN_SUBSTITUTIONS_{}' + ''.format(k.upper().replace('-', '__'))] = str(v) + env_subs['DATALAD_RUN_SUBSTITUTIONS_SPECPATH'] = rel_spec_path + env_subs['DATALAD_RUN_SUBSTITUTIONS_ANONYMIZE'] = str(anonymize) + + # TODO: The above two blocks to build replacements dict and + # env_subs should be joined eventually. + + for proc in procedure_list: + if heuristic.has_specval(proc, 'procedure-name'): + proc_name = heuristic.get_specval(proc, 'procedure-name') + else: + # invalid procedure spec + lgr.warning("conversion procedure missing key " + "'procedure-name' in %s: %s", + spec_path, proc) + # TODO: continue or yield impossible/error so it can be + # dealt with via on_failure? + continue + + if proc_name == 'ignore': + continue + + proc_call = heuristic.get_specval(proc, 'procedure-call') \ + if heuristic.has_specval(proc, 'procedure-call') \ + else None + + only_once = heuristic.get_specval(proc, + 'once-per-acquisition') \ + if heuristic.has_specval(proc, 'once-per-acquisition') \ + else None + + if only_once and ran_procedure.get(proc_name, False): + # if it wants to run only once per acquisition and we + # already ran it, don't call it again + continue + + # if spec comes with call format string, it takes precedence + # over what is generally configured for the procedure + # TODO: Not sure yet whether this is how we should deal with it + if proc_call: + env_subs['DATALAD.PROCEDURES.{}.CALL-FORMAT' + ''.format(proc_name)] = proc_call - # special treatment of DICOMs (using heudiconv) - # But it's one call to heudiconv for all DICOMs of an - # acquisition! + run_results = list() + # Note, that we can't use dataset.config.overrides to + # pass run-substitution config to procedures, since we + # leave python context and thereby loose the dataset + # instance. Use patched os.environ instead. Note also, + # that this requires names of substitutions to not + # contain underscores, since they would be translated to + # '.' by ConfigManager when reading them from within the + # procedure's datalad-run calls. from mock import patch - from tempfile import mkdtemp - # relative path to not-needed-heudiconv output: - rel_trash_path = relpath(mkdtemp(prefix="hirni-tmp-", - dir=opj(dataset.path, - ".git")), - dataset.path) - run_results = list() - with patch.dict('os.environ', - {'HIRNI_STUDY_SPEC': rel_spec_path, - 'HIRNI_SPEC2BIDS_SUBJECT': replacements['bids_subject']}): - - for r in dataset.containers_run( - ['heudiconv', - # XXX absolute path will make rerun on other - # system impossible -- hard to avoid - '-f', heuristic.__file__, - # leaves identifying info in run record - '-s', replacements['bids_subject'], - '-c', 'dcm2niix', - # TODO decide on the fate of .heudiconv/ - # but ATM we need to (re)move it: - # https://github.com/nipy/heudiconv/issues/196 - '-o', rel_trash_path, - '-b', - '-a', '{dspath}', - '-l', '', - # avoid glory details provided by dcmstack, - # we have them in the aggregated DICOM - # metadata already - '--minmeta', - '--files', replacements['location'] - ], - sidecar=anonymize, - container_name=dataset.config.get( - "datalad.hirni.conversion-container", - "conversion"), - inputs=[replacements['location'], rel_spec_path], - outputs=[dataset.path], - message="[HIRNI] Convert DICOM data for subject {}" - "".format(replacements['bids_subject']), - return_type='generator', + # TODO: Reconsider that patching. Shouldn't it be an update? + with patch.dict('os.environ', env_subs): + # apparently reload is necessary to consider config + # overrides via env: + dataset.config.reload() + for r in dataset.run_procedure( + spec='hirni-dicom-converter', + return_type='generator' ): - # if there was an issue with containers-run, - # yield original result, otherwise swallow: - if r['status'] not in ['ok', 'notneeded']: - yield r + # # if there was an issue yield original result, + # # otherwise swallow: + # if r['status'] not in ['ok', 'notneeded']: + yield r run_results.append(r) if not all(r['status'] in ['ok', 'notneeded'] for r in run_results): - yield {'action': 'heudiconv', + yield {'action': proc_name, 'path': spec_path, 'snippet': spec_snippet, 'status': 'error', @@ -213,100 +254,61 @@ def __call__(specfile, dataset=None, anonymize=False): "See previous message(s)."} else: - yield {'action': 'heudiconv', + yield {'action': proc_name, 'path': spec_path, 'snippet': spec_snippet, 'status': 'ok', 'message': "acquisition converted."} - # remove superfluous heudiconv output - rmtree(opj(dataset.path, rel_trash_path)) - # remove empty *_events.tsv files created by heudiconv - import glob - dataset.remove(glob.glob('*/*/*_events.tsv'), - check=False, - message="[HIRNI] Remove empty *_event.tsv " - "files") - - # run heudiconv only once - ran_heudiconv = True - - elif heuristic.has_specval(spec_snippet, 'converter') and \ - heuristic.get_specval(spec_snippet, 'converter') != 'heudiconv': - # Spec snippet comes with a specific converter call. - - # TODO: RF: run_converter() - - if 'converter-container' in spec_snippet and spec_snippet['converter-container']['value']: - from functools import partial - run_cmd = partial( - dataset.containers_run, - container_name=spec_snippet['converter-container']['value'] - ) - - else: - run_cmd = dataset.run - - run_results = list() - for r in run_cmd( - spec_snippet['converter']['value'], - sidecar=anonymize, - inputs=[replacements['location'], rel_spec_path], - outputs=[dataset.path], - # Note: The following message construction is - # supposed to not include the acquisition identifier - # if --anonymize was given, since it might contain - # the original subject ID. - message="Convert {} for subject {}".format( - spec_snippet['type'], - replacements['bids_subject']), - return_type='generator', - # - ): - - # if there was an issue with containers-run, - # yield original result, otherwise swallow: - if r['status'] not in ['ok', 'notneeded']: - yield r - - run_results.append(r) - - if not all(r['status'] in ['ok', 'notneeded'] - for r in run_results): - yield {'action': 'spec2bids', - 'path': spec_path, - 'snippet': spec_snippet, - 'status': 'error', - 'message': "Conversion failed. " - "See previous message(s)."} - - else: - yield {'action': 'specsnippet2bids', - 'path': spec_path, - 'snippet': spec_snippet, - 'status': 'ok', - 'message': "specification converted."} - - else: - if heuristic.has_specval(spec_snippet, 'converter') and \ - heuristic.get_specval(spec_snippet, 'converter') == 'heudiconv' and \ - ran_heudiconv: - # in this case we acted upon this snippet already and - # do not have to produce a result - pass - else: - # no converter specified in this snippet or it's a - # dicomseries and heudiconv was called already - # => nothing to do here. - yield get_status_dict( - action='spec2bids', - path=spec_path, - snippet=spec_snippet, - status='notneeded', - ) + # mark as a procedure we ran on this acquisition: + ran_procedure[proc_name] = True + + # elif proc_name != 'hirni-dicom-converter': + # # specific converter procedure call + # + # from mock import patch + # with patch.dict('os.environ', env_subs): + # # apparently reload is necessary to consider config + # # overrides via env: + # dataset.config.reload() + # + # for r in dataset.run_procedure( + # spec=[proc_name, rel_spec_path, anonymize], + # return_type='generator' + # ): + # + # # if there was an issue with containers-run, + # # yield original result, otherwise swallow: + # if r['status'] not in ['ok', 'notneeded']: + # yield r + # + # run_results.append(r) + # + # if not all(r['status'] in ['ok', 'notneeded'] + # for r in run_results): + # yield {'action': proc_name, + # 'path': spec_path, + # 'snippet': spec_snippet, + # 'status': 'error', + # 'message': "Conversion failed. " + # "See previous message(s)."} + # + # else: + # yield {'action': proc_name, + # 'path': spec_path, + # 'snippet': spec_snippet, + # 'status': 'ok', + # 'message': "specification converted."} + + # elif ran_heudiconv and proc_name == 'hirni-dicom-converter': + # # in this case we acted upon this snippet already and + # # do not have to produce a result + # pass + # + # else: + # # this shouldn't happen! + # raise RuntimeError yield {'action': 'spec2bids', 'path': spec_path, 'status': 'ok'} - - diff --git a/datalad_hirni/commands/spec4anything.py b/datalad_hirni/commands/spec4anything.py index 2f10e5f6..fca5e779 100644 --- a/datalad_hirni/commands/spec4anything.py +++ b/datalad_hirni/commands/spec4anything.py @@ -24,7 +24,7 @@ # TODO: Prob. should be (partially?) editable, but for now we need consistency # here: -non_editables = ['location', 'type', 'dataset_id', 'dataset_refcommit'] +non_editables = ['location', 'type', 'dataset-id', 'dataset-refcommit'] def _get_edit_dict(value=None, approved=False): @@ -51,8 +51,8 @@ def _add_to_spec(spec, spec_dir, path, meta, overrides=None, replace=False): snippet = { 'type': 'generic_' + path['type'], 'location': posixpath.relpath(path['path'], spec_dir), - 'dataset_id': meta['dsid'], - 'dataset_refcommit': meta['refcommit'], + 'dataset-id': meta['dsid'], + 'dataset-refcommit': meta['refcommit'], 'id': _get_edit_dict(), 'converter': _get_edit_dict(), 'comment': _get_edit_dict(value=""), diff --git a/datalad_hirni/resources/webapp/specedit.html b/datalad_hirni/resources/webapp/specedit.html index 5214cc63..8d73eac7 100644 --- a/datalad_hirni/resources/webapp/specedit.html +++ b/datalad_hirni/resources/webapp/specedit.html @@ -66,7 +66,7 @@ Common properties (record: {{ studyspecfile }})
-
+
@@ -85,12 +85,12 @@ {{ studyspecfile }}/{{ spec.location }} [ {{ spec.type }}; {{ spec.uid }} ]
    -
  • +
  • -
  • +
  • @@ -148,10 +148,9 @@ specfiles: [], studyspecfile: null, field_purpose: { - comment: "Comments or notes for this record", - converter: "Command to call for converting data associated with this record to BIDS format, may use template syntax and placeholders", - converter_path: "Location of a program/script to use as converter command for this record, value can be used in converter command via {_hs[converter_path]} placeholder", - description: "Record description, originally extracted from the source file(s)" + comment: "Comments or notes for this record", + converter: "Procedures to call for converting data associated with this record to BIDS format, may use template syntax and placeholders", + description: "Record description, originally extracted from the source file(s)" } }, methods: { diff --git a/datalad_hirni/support/dicom2bids_rules.py b/datalad_hirni/support/dicom2bids_rules.py index 090593bb..2782ad46 100644 --- a/datalad_hirni/support/dicom2bids_rules.py +++ b/datalad_hirni/support/dicom2bids_rules.py @@ -7,10 +7,10 @@ # subjects to the rule set # XXX this is not used at all spec_keys = [ - 'bids_session', - 'bids_task', - 'bids_run', - 'bids_modality', + 'bids-session', + 'bids-task', + 'bids-run', + 'bids-modality', 'comment', 'converter', 'description', @@ -131,19 +131,19 @@ def _rules(self, record, subject=None, anon_subject=None, session=None): 'description': record['SeriesDescription'] if "SeriesDescription" in record else '', 'comment': '', 'subject': apply_bids_label_restrictions(_guess_subject(record) if not subject else subject), - 'anon_subject': apply_bids_label_restrictions(anon_subject) if anon_subject else None, - 'bids_session': apply_bids_label_restrictions(_guess_session(record) if not session else session), - 'bids_task': apply_bids_label_restrictions(_guess_task(record)), - 'bids_run': apply_bids_label_restrictions(run) if run else str(self.runs[protocol_name]), - 'bids_modality': apply_bids_label_restrictions(_guess_modality(record)), + 'anon-subject': apply_bids_label_restrictions(anon_subject) if anon_subject else None, + 'bids-session': apply_bids_label_restrictions(_guess_session(record) if not session else session), + 'bids-task': apply_bids_label_restrictions(_guess_task(record)), + 'bids-run': apply_bids_label_restrictions(run) if run else str(self.runs[protocol_name]), + 'bids-modality': apply_bids_label_restrictions(_guess_modality(record)), # TODO: No defaults yet (May be there shouldn't be defaults, but # right now, that's not a conscious decision ...): - 'bids_acquisition': apply_bids_label_restrictions(None), #acq - 'bids_contrast_enhancement': apply_bids_label_restrictions(None), # ce - 'bids_reconstruction_algorithm': apply_bids_label_restrictions(None), #rec - 'bids_echo': apply_bids_label_restrictions(None), #echo - 'bids_direction': apply_bids_label_restrictions(None), #dir + 'bids-acquisition': apply_bids_label_restrictions(None), #acq + 'bids-contrast-enhancement': apply_bids_label_restrictions(None), # ce + 'bids-reconstruction-algorithm': apply_bids_label_restrictions(None), #rec + 'bids-echo': apply_bids_label_restrictions(None), #echo + 'bids-direction': apply_bids_label_restrictions(None), #dir 'id': record.get('SeriesNumber', None), } @@ -243,6 +243,10 @@ def _guess_modality(record): return "t2w" # END + # TEMP: Reproin workaround + if "func" in prot_parts: + return "bold" + # found nothing, but modality isn't necessarily required return None diff --git a/datalad_hirni/support/hirni_heuristic.py b/datalad_hirni/support/hirni_heuristic.py index e97e295a..b5a73106 100644 --- a/datalad_hirni/support/hirni_heuristic.py +++ b/datalad_hirni/support/hirni_heuristic.py @@ -59,17 +59,17 @@ # map specification keys to BIDS abbreviation used in paths spec2bids_map = { 'subject': "sub", - 'anon_subject': "sub", - 'bids_session': "ses", - 'bids_task': "task", - 'bids_run': "run", - 'bids_modality': "mod", - 'bids_acquisition': "acq", - 'bids_scan': "scan", - 'bids_contrast_enhancement': "ce", - 'bids_reconstruction_algorithm': "rec", - 'bids_echo': "echo", - 'bids_direction': "dir", + 'anon-subject': "sub", + 'bids-session': "ses", + 'bids-task': "task", + 'bids-run': "run", + 'bids-modality': "mod", + 'bids-acquisition': "acq", + 'bids-scan': "scan", + 'bids-contrast-enhancement': "ce", + 'bids-reconstruction-algorithm': "rec", + 'bids-echo': "echo", + 'bids-direction': "dir", } @@ -160,22 +160,22 @@ def validate_spec(spec): raise ValueError("Image series specification is empty.") # check converter - converter = get_specval(spec, 'converter') - if converter == 'ignore': - lgr.debug("Skip series %s (marked 'ignore' in spec)", spec['uid']) - return False - if converter != 'heudiconv': - lgr.debug("Skip series %s since it's not supposed to be converted by " - "heudiconv.", spec['uid']) - return False + # converter = get_specval(spec, 'converter') + # if converter == 'ignore': + # lgr.debug("Skip series %s (marked 'ignore' in spec)", spec['uid']) + # return False + # if converter != 'heudiconv': + # lgr.debug("Skip series %s since it's not supposed to be converted by " + # "heudiconv.", spec['uid']) + # return False # mandatory keys for any spec dict (not only dicomseries) for k in spec.keys(): # automatically managed keys with no subdict: # TODO: Where to define this list? # TODO: Test whether those are actually present! - if k in ['type', 'status', 'location', 'uid', 'dataset_id', - 'dataset_refcommit']: + if k in ['type', 'location', 'uid', 'dataset-id', + 'dataset-refcommit', 'converter']: continue if 'value' not in spec[k]: lgr.warning("DICOM series specification (UID: {uid}) has no value " @@ -190,7 +190,7 @@ def validate_spec(spec): lgr.warning("Missing image series UID.") return False - for var in ('bids_modality',): + for var in ('bids-modality',): if not has_specval(spec, var): lgr.warning("Missing specification value for key '%s'", var) return False @@ -235,13 +235,13 @@ def infotodict(seqinfo): # pragma: no cover dirname = filename = "sub-{}".format(_spec.subject) # session - if has_specval(series_spec, 'bids_session'): - ses = get_specval(series_spec, 'bids_session') + if has_specval(series_spec, 'bids-session'): + ses = get_specval(series_spec, 'bids-session') dirname += "/ses-{}".format(ses) filename += "_ses-{}".format(ses) # data type - modality = get_specval(series_spec, 'bids_modality') + modality = get_specval(series_spec, 'bids-modality') # make cannonical if possible modality = modality_label_map.get(modality, modality) # apply fixed mapping from modality -> data_type @@ -257,9 +257,9 @@ def infotodict(seqinfo): # pragma: no cover # func/sub-[_ses-] # _task-[_acq-