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 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ffd0d8e
Lock travis Python 0.6.5
adelavega Oct 9, 2018
8db5111
Merge master
adelavega Oct 15, 2018
801b295
Update BIDSDataGrabber for pybids 0.7, including tests, and derivativ…
adelavega Oct 15, 2018
1d267c8
Fix test, outfields
adelavega Oct 15, 2018
b0121c0
Refer to proper input, index_derivatives
adelavega Oct 16, 2018
cc89c58
Datatype not modality, in pybids tests
adelavega Oct 19, 2018
b8aacce
Merge master
adelavega Jan 11, 2019
a0e92f1
Lint BIDSGrabbder
adelavega Jan 11, 2019
0f94d9d
Bump pybids version to 0.7.0
adelavega Jan 11, 2019
77a0510
use https for travis pybids requirement
adelavega Jan 11, 2019
26a57a6
Merge branch 'master' into update-pybids
adelavega Jan 14, 2019
2a84cab
Merge branch 'master' into update-pybids
adelavega Jan 14, 2019
5590d4d
Re-enable editable mode, and run tests in Python 2
adelavega Jan 14, 2019
3c21466
Merge branch 'master' of https://github.com/nipy/nipype into update-p…
adelavega Jan 16, 2019
d2e53fb
Merge master and fix merge conflict
adelavega Jan 22, 2019
b75bf7c
Update nipype/info.py
effigies Jan 22, 2019
7105cb5
Remove return type named tuple
adelavega Jan 22, 2019
09a1a43
Merge branch 'update-pybids' of github.com:adelavega/nipype into upda…
adelavega Jan 22, 2019
75a5a2b
Update nipype/interfaces/io.py
effigies Jan 22, 2019
139c163
Merge branch 'update-pybids' of github.com:adelavega/nipype into upda…
adelavega Jan 22, 2019
b623cdd
remake specs
adelavega Jan 22, 2019
aeda06a
Add FSL auto test
adelavega Jan 22, 2019
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 @@ -86,8 +86,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 git+https://github.com/bids-standard/pybids.git@0.7.0#egg=pybids
Copy link
Member

Choose a reason for hiding this comment

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

Tests skip unless it's installed in editable mode.

interfaces/tests/test_io.py::test_bids_grabber 
[gw0] [ 83%] SKIPPED interfaces/tests/test_io.py::test_bids_grabber 
interfaces/tests/test_io.py::test_bids_fields 
[gw0] [ 84%] SKIPPED interfaces/tests/test_io.py::test_bids_fields 
interfaces/tests/test_io.py::test_bids_infields_outfields 
[gw0] [ 84%] SKIPPED interfaces/tests/test_io.py::test_bids_infields_outfields 

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 is by design because when you install in editable mode you get the data that is needed for the tests

Copy link
Member

Choose a reason for hiding this comment

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

Right. So we need to switch back to editable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I thought it was editable, not sure why that changed.


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 @@ -92,7 +92,7 @@ function generate_main_dockerfile() {
conda_install='python=${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}
icu=58.1 libxml2 libxslt matplotlib mkl numpy paramiko
pandas psutil scikit-learn scipy traits=4.6.0' \
pip_install="grabbit==0.1.2 https://github.com/INCF/pybids/tarball/0.6.5" \
pip_install="grabbit==0.2.6 https://github.com/bids-standard/pybids/tarball/0.7.0" \
Copy link
Member

Choose a reason for hiding this comment

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

This is going to have conflicts when #2856 is merged. Also, for tests to work, this will need to be installed in editable mode. Perhaps add https://github.com/bids-standard/pybids/tarball/0.7.0 to L110:

-                pip_install="/src/nipype[all]" \
+                pip_install="/src/nipype[all] https://github.com/bids-standard/pybids/tarball/0.7.0" \

activate=true \
--copy docker/files/run_builddocs.sh docker/files/run_examples.sh \
docker/files/run_pytests.sh nipype/external/fsl_imglob.py /usr/bin/ \
Expand Down
2 changes: 1 addition & 1 deletion nipype/info.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,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: 34 additions & 31 deletions nipype/interfaces/io.py
Original file line number Diff line number Diff line change
Expand Up @@ -2721,18 +2721,25 @@ 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')
return_type = traits.Enum(
'file', 'namedtuple', usedefault=True)
Copy link
Member

Choose a reason for hiding this comment

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

Have we tested return_type == 'namedtuple' anywhere? I think this went away in 0.7.0. If we're going to drop backwards compatibility, it might be worth removing this one.

Copy link
Contributor

@jdkent jdkent Jan 13, 2019

Choose a reason for hiding this comment

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

I tested return_type=namedtuple locally with pybids 0.7.0 installed and adelavega's branch of nipype:
code:

from nipype.interfaces.io import BIDSDataGrabber
%%bash
datalad install ///openneuro/ds000114
bids_grabber = BIDSDataGrabber(base_dir='ds000114', return_type='namedtuple')
res = bids_grabber.run()

I got the following error traceback:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-23-001916242d69> in <module>
----> 1 res = bids_grabber.run()

~/.conda/envs/bids_nipy/lib/python3.6/site-packages/nipype/interfaces/base/core.py in run(self, cwd, ignore_exception, **inputs)
    369             runtime = self._run_interface(runtime)
    370             runtime = self._post_run_hook(runtime)
--> 371             outputs = self.aggregate_outputs(runtime)
    372         except Exception as e:
    373             import traceback

~/.conda/envs/bids_nipy/lib/python3.6/site-packages/nipype/interfaces/base/core.py in aggregate_outputs(self, runtime, needed_outputs)
    447         """
    448 
--> 449         predicted_outputs = self._list_outputs()
    450         outputs = self._outputs()
    451         if predicted_outputs:

~/.conda/envs/bids_nipy/lib/python3.6/site-packages/nipype/interfaces/io.py in _list_outputs(self)
   2832             args = query.copy()
   2833             args.update(filters)
-> 2834             filelist = layout.get(return_type=self.inputs.return_type, **args)
   2835             if len(filelist) == 0:
   2836                 msg = 'Output key: %s returned no files' % key

~/.conda/envs/bids_nipy/lib/python3.6/site-packages/bids/layout/layout.py in get(self, return_type, target, extensions, derivatives, regex_search, defined_fields, domains, **kwargs)
    422             BIDSLayout, self).get(return_type, target=target,
    423                                   extensions=extensions, domains=None,
--> 424                                   regex_search=regex_search, **ent_kwargs)
    425 
    426         # Search the metadata if needed

~/.conda/envs/bids_nipy/lib/python3.6/site-packages/grabbit/core.py in get(self, return_type, target, extensions, domains, regex_search, **kwargs)
    761 
    762             if target is None:
--> 763                 raise ValueError('If return_type is "id" or "dir", a valid '
    764                                  'target entity must also be specified.')
    765             result = [x for x in result if target in x.entities]

ValueError: If return_type is "id" or "dir", a valid target entity must also be specified.

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'll remove that

Copy link
Member

Choose a reason for hiding this comment

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

You were going to remove namedtuple?

index_derivatives = traits.Bool(
False, 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 +2765,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 +2788,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 +2813,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 Down
10 changes: 5 additions & 5 deletions nipype/interfaces/tests/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -591,9 +591,9 @@ 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...

Expand All @@ -607,7 +607,7 @@ def test_bids_fields(tmpdir):
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)

Expand All @@ -633,9 +633,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