Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --skiptop flag to prevent writing top level bids files #344

Merged
merged 7 commits into from
May 21, 2019
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion docs/usage.rst
Original file line number Diff line number Diff line change
@@ -43,6 +43,13 @@ DICOMs as an independent ``heudiconv`` execution.
The first script aggregates the DICOM directories and submits them to
``run_heudiconv.sh`` with SLURM as a job array.

If using bids, the ``notop`` bids option suppresses creation of
top-level files in the bids directory (e.g.,
``dataset_description.json``) to avoid possible race conditions.
These files may be generated later with ``populate_templates.sh``
below (except for ``participants.tsv``, which must be create
manually).

.. code:: shell

#!/bin/bash
@@ -76,7 +83,22 @@ The second script processes a DICOM directory with ``heudiconv`` using the built
echo Submitted directory: ${DCMDIR}

IMG="/singularity-images/heudiconv-0.5.4-dev.sif"
CMD="singularity run -B ${DCMDIR}:/dicoms:ro -B ${OUTDIR}:/output -e ${IMG} --files /dicoms/ -o /output -f reproin -c dcm2niix -b --minmeta -l ."
CMD="singularity run -B ${DCMDIR}:/dicoms:ro -B ${OUTDIR}:/output -e ${IMG} --files /dicoms/ -o /output -f reproin -c dcm2niix -b notop --minmeta -l ."

printf "Command:\n${CMD}\n"
${CMD}
echo "Successful process"

This script creates the top-level bids files (e.g.,
``dataset_description.json``)

..code:: shell
#!/bin/bash
set -eu

OUTDIR=${1}
IMG="/singularity-images/heudiconv-0.5.4-dev.sif"
CMD="singularity run -B ${OUTDIR}:/output -e ${IMG} --files /output -f reproin --command populate-templates"

printf "Command:\n${CMD}\n"
${CMD}
40 changes: 37 additions & 3 deletions heudiconv/cli/run.py
Original file line number Diff line number Diff line change
@@ -18,6 +18,10 @@

INIT_MSG = "Running {packname} version {version}".format

BIDS_OPTIONS = [('notop', 'Skip creating of top-level bids files. '
'Useful when running in batch mode to prevent '
'possible race conditions.')]


def is_interactive():
"""Return True if all in/outs are tty"""
@@ -98,9 +102,20 @@ def process_extra_commands(outdir, args):
return


def help_bids():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be added to the argument's desc field instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added bids options information to the help field (as far as I can tell desc is only parser level).

sys.stderr.write('bids specific options can be passed after the bids flag.\n'
'For example, "--bids notop".\n'
'The currently supported options are:\n')
for option, helpstr in BIDS_OPTIONS:
sys.stderr.write('{}: {}\n'.format(option, helpstr))


def main(argv=None):
parser = get_parser()
args = parser.parse_args(argv)
if args.help_bids:
help_bids()
sys.exit(1)
# exit if nothing to be done
if not args.files and not args.dicom_dir_template and not args.command:
lgr.warning("Nothing to be done - displaying usage help")
@@ -112,6 +127,19 @@ def main(argv=None):
random.seed(args.random_seed)
import numpy
numpy.random.seed(args.random_seed)
# Ensure only supported bids options are passed
allowed_options = [option for option, _ in BIDS_OPTIONS]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argparse's choices will check if each input is valid, so this isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if args.bids is not None:
for bids_option in args.bids:
if bids_option not in allowed_options:
lgr.warning("{} is not a valid bids option - displaying bids options help".format(bids_option))
help_bids()
sys.exit(1)
args.bids_options = args.bids
args.bids = True
else:
args.bids_options = []
args.bids = False
if args.debug:
lgr.setLevel(logging.DEBUG)
# Should be possible but only with a single subject -- will be used to
@@ -181,8 +209,11 @@ def get_parser():
parser.add_argument('-ss', '--ses', dest='session', default=None,
help='session for longitudinal study_sessions, default '
'is none')
parser.add_argument('-b', '--bids', action='store_true',
help='flag for output into BIDS structure')
parser.add_argument('-b', '--bids', nargs='*',
metavar=('BIDSOPTION1', 'BIDSOPTION2'),
help='flag for output into BIDS structure. '
'Can also take bids specific options. Use --help-bids '
'for more information.')
parser.add_argument('--overwrite', action='store_true', default=False,
help='flag to allow overwriting existing converted files')
parser.add_argument('--datalad', action='store_true',
@@ -222,6 +253,8 @@ def get_parser():
help='Additional queue arguments passed as '
'single string of Argument=Value pairs space '
'separated.')
parser.add_argument('--help-bids', action='store_true', dest='help_bids',
help='Display bids specific help.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to just add "help" option to --bids and react on it? part of the goal in my suggestion was not to expand the cmdline API options list more ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes that's a good ideal. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored as per @mgxd's suggestion and just included all the help in the argparse help field

return parser


@@ -320,7 +353,8 @@ def process_args(args):
seqinfo=seqinfo,
min_meta=args.minmeta,
overwrite=args.overwrite,
dcmconfig=args.dcmconfig,)
dcmconfig=args.dcmconfig,
bids_options=args.bids_options,)

lgr.info("PROCESSING DONE: {0}".format(
str(dict(subject=sid, outdir=study_outdir, session=session))))
4 changes: 2 additions & 2 deletions heudiconv/convert.py
Original file line number Diff line number Diff line change
@@ -80,7 +80,7 @@ def conversion_info(subject, outdir, info, filegroup, ses):

def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
anon_outdir, with_prov, ses, bids, seqinfo, min_meta,
overwrite, dcmconfig):
overwrite, dcmconfig, bids_options):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is what I was hoping to avoid -- needing to eventually add/adjust signature of every function which now gets bids but might eventually need to get access to bids_options...

but may be making it more obvious at the cost of some duplication would be better than my more obscure suggestion... Let's see what others thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see what you're saying. I wanted to keep bids as a boolean because I think it's more obvious to people and prevents us having to check every usage of args.bids. (e.g., if bids is an empty list, we would want to calculate bids stuff, but if args.bids: would no longer work.) Therefore a change is required in either case, and I feel the bids + bids_options method is more explicit and clear. I'm curious what others thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd like to avoid additional parameters to the signatures whenever possible. Though if we're changing the variable type + intention, it may be worthwhile to rename it to something more readable - I like bids_options or bids_opts. Most of the current checks can be ported rather easily

if args.bids_opts is not None:  # if args.bids
   if ['notopdirs'] in args.bids_opts:  # special case
       ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to use bids_options internally

if dicoms:
lgr.info("Processing %d dicoms", len(dicoms))
elif seqinfo:
@@ -201,7 +201,7 @@ def prep_conversion(sid, dicoms, outdir, heuristic, converter, anon_sid,
for item_dicoms in filegroup.values():
clear_temp_dicoms(item_dicoms)

if bids:
if bids and 'notop' not in bids_options:
if seqinfo:
keys = list(seqinfo)
add_participant_record(anon_outdir,