Skip to content

[ENH] Update BIDSDataGrabber for pybids 0.7 #2737

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

Merged
merged 22 commits into from
Jan 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ before_install:
fi;

- travis_retry pip install -r requirements.txt
- travis_retry pip install grabbit==0.1.2
- travis_retry git clone -b 0.6.5 https://github.com/INCF/pybids.git ${HOME}/pybids && pip install -e ${HOME}/pybids
- travis_retry pip install grabbit==0.2.6
- travis_retry pip install -e git+https://github.com/bids-standard/pybids.git@0.7.0#egg=pybids

install:
- travis_retry pip install $EXTRA_PIP_FLAGS -e .[$NIPYPE_EXTRAS]
Expand Down
2 changes: 1 addition & 1 deletion docker/generate_dockerfiles.sh
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ function generate_main_dockerfile() {
--user neuro \
--miniconda use_env=neuro \
pip_opts="-e" \
pip_install="/src/nipype[all]" \
pip_install="/src/nipype[all] https://github.com/bids-standard/pybids/tarball/0.7.0" \
--workdir /work \
--label org.label-schema.build-date='$BUILD_DATE' \
org.label-schema.name="NIPYPE" \
Expand Down
2 changes: 1 addition & 1 deletion nipype/info.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def get_nipype_gitversion():
'profiler': ['psutil>=5.0'],
'duecredit': ['duecredit'],
'xvfbwrapper': ['xvfbwrapper'],
'pybids': ['pybids==0.6.5'],
'pybids': ['pybids>=0.7.0'],
'ssh': ['paramiko'],
# 'mesh': ['mayavi'] # Enable when it works
}
Expand Down
65 changes: 65 additions & 0 deletions nipype/interfaces/fsl/tests/test_auto_EddyQuad.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# AUTO-GENERATED by tools/checkspecs.py - DO NOT EDIT
from __future__ import unicode_literals
from ..epi import EddyQuad


def test_EddyQuad_inputs():
input_map = dict(
args=dict(argstr='%s', ),
base_name=dict(
argstr='%s',
position=0,
usedefault=True,
),
bval_file=dict(
argstr='--bvals=%s',
mandatory=True,
),
bvec_file=dict(argstr='--bvecs=%s', ),
environ=dict(
nohash=True,
usedefault=True,
),
field=dict(argstr='--field=%s', ),
idx_file=dict(
argstr='--eddyIdx=%s',
mandatory=True,
),
mask_file=dict(
argstr='--mask=%s',
mandatory=True,
),
output_dir=dict(
argstr='--output-dir=%s',
name_source=['base_name'],
name_template='%s.qc',
),
output_type=dict(),
param_file=dict(
argstr='--eddyParams=%s',
mandatory=True,
),
slice_spec=dict(argstr='--slspec=%s', ),
verbose=dict(argstr='--verbose', ),
)
inputs = EddyQuad.input_spec()

for key, metadata in list(input_map.items()):
for metakey, value in list(metadata.items()):
assert getattr(inputs.traits()[key], metakey) == value
def test_EddyQuad_outputs():
output_map = dict(
avg_b0_pe_png=dict(),
avg_b_png=dict(),
clean_volumes=dict(),
cnr_png=dict(),
qc_json=dict(),
qc_pdf=dict(),
residuals=dict(),
vdm_png=dict(),
)
outputs = EddyQuad.output_spec()

for key, metadata in list(output_map.items()):
for metakey, value in list(metadata.items()):
assert getattr(outputs.traits()[key], metakey) == value
65 changes: 33 additions & 32 deletions nipype/interfaces/io.py
Original file line number Diff line number Diff line change
Expand Up @@ -2721,18 +2721,23 @@ def _list_outputs(self):


class BIDSDataGrabberInputSpec(DynamicTraitedSpec):
base_dir = Directory(exists=True,
desc='Path to BIDS Directory.',
mandatory=True)
output_query = traits.Dict(key_trait=Str,
value_trait=traits.Dict,
desc='Queries for outfield outputs')
raise_on_empty = traits.Bool(True, usedefault=True,
desc='Generate exception if list is empty '
'for a given field')
return_type = traits.Enum('file', 'namedtuple', usedefault=True)
strict = traits.Bool(desc='Return only BIDS "proper" files (e.g., '
'ignore derivatives/, sourcedata/, etc.)')
base_dir = Directory(
exists=True,
desc='Path to BIDS Directory.',
mandatory=True)
output_query = traits.Dict(
key_trait=Str,
value_trait=traits.Dict,
desc='Queries for outfield outputs')
raise_on_empty = traits.Bool(
True, usedefault=True,
desc='Generate exception if list is empty for a given field')
index_derivatives = traits.Bool(
False, mandatory=True, usedefault=True,
desc='Index derivatives/ sub-directory')
Copy link
Member

Choose a reason for hiding this comment

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

This should be mandatory=True, since you don't use an isdefined() check on it below. It's possible to pass an Undefined to things with defaults to disable the input, unless mandatory is set True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok!

extra_derivatives = traits.List(
Copy link
Contributor

Choose a reason for hiding this comment

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

when using the add_derivatives method, the docstring indicates this can either be a list or a string.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extra_derivatives = traits.List(
extra_derivatives = InputMultiObject(

(Requires importing ImportMultiObject along with InputMultiPath; we should move all *MultiPaths to *MultiObjects, but that shouldn't be this PR.)

Directory(exists=True),
desc='Additional derivative directories to index')


class BIDSDataGrabber(LibraryBaseInterface, IOBase):
Expand All @@ -2758,10 +2763,10 @@ class BIDSDataGrabber(LibraryBaseInterface, IOBase):
are filtered on common entities, which can be explicitly defined as
infields.

>>> bg = BIDSDataGrabber(infields = ['subject'], outfields = ['dwi'])
>>> bg = BIDSDataGrabber(infields = ['subject'])
>>> bg.inputs.base_dir = 'ds005/'
>>> bg.inputs.subject = '01'
>>> bg.inputs.output_query['dwi'] = dict(modality='dwi')
>>> bg.inputs.output_query['dwi'] = dict(datatype='dwi')
>>> results = bg.run() # doctest: +SKIP

"""
Expand All @@ -2781,18 +2786,17 @@ def __init__(self, infields=None, **kwargs):

if not isdefined(self.inputs.output_query):
self.inputs.output_query = {
"func": {"modality": "func", 'extensions': ['nii', '.nii.gz']},
"anat": {"modality": "anat", 'extensions': ['nii', '.nii.gz']},
"bold": {"datatype": "func", "suffix": "bold",
"extensions": ["nii", ".nii.gz"]},
"T1w": {"datatype": "anat", "suffix": "T1w",
"extensions": ["nii", ".nii.gz"]},
}

# If infields is empty, use all BIDS entities
if infields is None:
# Version resilience
try:
from bids import layout as bidslayout
except ImportError:
from bids import grabbids as bidslayout
bids_config = join(dirname(bidslayout.__file__), 'config', 'bids.json')
from bids import layout as bidslayout
bids_config = join(
dirname(bidslayout.__file__), 'config', 'bids.json')
bids_config = json.load(open(bids_config, 'r'))
infields = [i['name'] for i in bids_config['entities']]
Copy link
Member

Choose a reason for hiding this comment

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

Can we replace all of this with BIDSLayout().entities? I'm not a fan of digging through the directory structure of a package unless there are no other options.

Copy link
Contributor

@jdkent jdkent Jan 14, 2019

Choose a reason for hiding this comment

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

That command (BIDSLayout().entities) returned an error for me:
TypeError: __init__() missing 1 required positional argument: 'root'.

I do not know of a better way (but I didn't look extremely hard)

Another comment: I think it would be nice to include derivatives.json (or the derivatives entities) by default as well for queries to be maximally flexible.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I meant that in a pseudocode sense of the entities attribute of an instantiated object, not that that snippet would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, maybe I can reorganize this to set up the layout first, before setting the output traits.

Is there any problem with instantiating the layout in __init__?

Copy link
Member

Choose a reason for hiding this comment

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

The biggest problem is that inputs can change between instantiation and running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a pretty big problem!

So I think for now instantiating a layout just to get entities does not work. Adding a helper function to pybids and then changing this later is a better option.


Expand All @@ -2807,15 +2811,12 @@ def __init__(self, infields=None, **kwargs):
self.inputs.trait_set(trait_change_notify=False, **undefined_traits)

def _list_outputs(self):
# Version resilience
try:
from bids import BIDSLayout
except ImportError:
from bids.grabbids import BIDSLayout
exclude = None
if self.inputs.strict:
exclude = ['derivatives/', 'code/', 'sourcedata/']
layout = BIDSLayout(self.inputs.base_dir, exclude=exclude)
from bids import BIDSLayout
layout = BIDSLayout(self.inputs.base_dir,
derivatives=self.inputs.index_derivatives)

if isdefined(self.inputs.extra_derivatives):
layout.add_derivatives(self.inputs.extra_derivatives)

# If infield is not given nm input value, silently ignore
filters = {}
Expand All @@ -2828,7 +2829,7 @@ def _list_outputs(self):
for key, query in self.inputs.output_query.items():
args = query.copy()
args.update(filters)
filelist = layout.get(return_type=self.inputs.return_type, **args)
filelist = layout.get(return_type='file', **args)
if len(filelist) == 0:
msg = 'Output key: %s returned no files' % key
if self.inputs.raise_on_empty:
Expand Down
7 changes: 5 additions & 2 deletions nipype/interfaces/tests/test_auto_BIDSDataGrabber.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@
def test_BIDSDataGrabber_inputs():
input_map = dict(
base_dir=dict(mandatory=True, ),
extra_derivatives=dict(),
index_derivatives=dict(
mandatory=True,
usedefault=True,
),
output_query=dict(),
raise_on_empty=dict(usedefault=True, ),
return_type=dict(usedefault=True, ),
strict=dict(),
)
inputs = BIDSDataGrabber.input_spec()

Expand Down
16 changes: 5 additions & 11 deletions nipype/interfaces/tests/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,6 @@ def test_jsonsink(tmpdir, inputs_attributes):
# There are three reasons these tests will be skipped:
@pytest.mark.skipif(not have_pybids,
reason="Pybids is not installed")
@pytest.mark.skipif(sys.version_info < (3, 0),
reason="Pybids no longer supports Python 2")
@pytest.mark.skipif(not dist_is_editable('pybids'),
reason="Pybids is not installed in editable mode")
def test_bids_grabber(tmpdir):
Expand All @@ -591,31 +589,27 @@ def test_bids_grabber(tmpdir):
bg.inputs.base_dir = os.path.join(datadir, 'ds005')
bg.inputs.subject = '01'
results = bg.run()
assert 'sub-01_T1w.nii.gz' in map(os.path.basename, results.outputs.anat)
assert 'sub-01_T1w.nii.gz' in map(os.path.basename, results.outputs.T1w)
assert 'sub-01_task-mixedgamblestask_run-01_bold.nii.gz' in \
map(os.path.basename, results.outputs.func)
map(os.path.basename, results.outputs.bold)


@pytest.mark.skipif(not have_pybids,
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the @pytest.mark.skipif(sys.version_info < (3, 0), lines a couple below, and for the other tests, too? Pybids does still support Python 2, and we should make sure that it works, as long as it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess...

reason="Pybids is not installed")
@pytest.mark.skipif(sys.version_info < (3, 0),
reason="Pybids no longer supports Python 2")
@pytest.mark.skipif(not dist_is_editable('pybids'),
reason="Pybids is not installed in editable mode")
def test_bids_fields(tmpdir):
tmpdir.chdir()
bg = nio.BIDSDataGrabber(infields = ['subject'], outfields = ['dwi'])
bg.inputs.base_dir = os.path.join(datadir, 'ds005')
bg.inputs.subject = '01'
bg.inputs.output_query['dwi'] = dict(modality='dwi')
bg.inputs.output_query['dwi'] = dict(datatype='dwi')
results = bg.run()
assert 'sub-01_dwi.nii.gz' in map(os.path.basename, results.outputs.dwi)


@pytest.mark.skipif(not have_pybids,
reason="Pybids is not installed")
@pytest.mark.skipif(sys.version_info < (3, 0),
reason="Pybids no longer supports Python 2")
@pytest.mark.skipif(not dist_is_editable('pybids'),
reason="Pybids is not installed in editable mode")
def test_bids_infields_outfields(tmpdir):
Expand All @@ -633,9 +627,9 @@ def test_bids_infields_outfields(tmpdir):
for outfield in outfields:
assert(outfield in bg._outputs().traits())

# now try without defining outfields, we should get anat and func for free
# now try without defining outfields
bg = nio.BIDSDataGrabber()
for outfield in ['anat', 'func']:
for outfield in ['T1w', 'bold']:
assert outfield in bg._outputs().traits()


Expand Down