-
Notifications
You must be signed in to change notification settings - Fork 533
[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
Conversation
nipype/interfaces/io.py
Outdated
"bold": {"datatype": "func", "suffix": "bold", | ||
"extensions": ["nii", ".nii.gz"]}, | ||
"T1w": {"datatype": "anat", "suffix": "T1w", | ||
"extensions": ["nii", ".nii.gz"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem for this PR, but we may want to think about making it a much more generally usable default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Such as? you mean returning more things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Ideally something that will contain most, if not all, files, and can just be queried sensibly. I feel like specifying the query is more of a power-user move, so making sure there's a reasonable way to ask for the files most people will want would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me, although I worry about about performance if a bunch of unnecessary queries are made. Although the indexing itself is probably the slowest part, so it should probably be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, other fields you'd want? brain_mask
is another one I can think of for fmri. I would like to hear from ppl doing preprocessing and and DWI as I'm unfamiliar with what is typically expected.
Codecov Report
@@ Coverage Diff @@
## master #2737 +/- ##
==========================================
- Coverage 67.38% 64.02% -3.36%
==========================================
Files 341 339 -2
Lines 43474 43412 -62
Branches 5401 5398 -3
==========================================
- Hits 29294 27795 -1499
- Misses 13482 14544 +1062
- Partials 698 1073 +375
Continue to review full report at Codecov.
|
Hopefully the tests will pass now. Otherwise, any other reviews? Either way, waiting for |
Waiting for pybids 0.7 release. Need to re-test and update the pin when that happens. |
Looks like tests are passing, but building docs have failed. Not my fault, promise! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adelavega A few comments. @jdkent I'd still appreciate your input when you get time.
.travis.yml
Outdated
- 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
nipype/interfaces/io.py
Outdated
True, usedefault=True, | ||
desc='Generate exception if list is empty for a given field') | ||
return_type = traits.Enum( | ||
'file', 'namedtuple', usedefault=True) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove that
There was a problem hiding this comment.
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
?
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']] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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__
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
'file', 'namedtuple', usedefault=True) | ||
index_derivatives = traits.Bool( | ||
False, usedefault=True, | ||
desc='Index derivatives/ sub-directory') |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok!
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing this out using bids queries, this works great, but I couldn't get this interface to track my derivatives.
I think one issue was that I have an older run of fmriprep, and I can't add the outdated entities through this interface (e.g. variant
).
I would like to be able to add my own entities (perhaps through the config kwarg in BIDSLayout?)
Another issue appeared to be when I was trying to use add_derivatives
with one derivatives directory, it looked like pybids was trying to index all of derivatives (which in my case can sometimes be large and unruly, but I think this may an issue more appropriate for pybids itself).
There are also some minor comments.
nipype/interfaces/io.py
Outdated
True, usedefault=True, | ||
desc='Generate exception if list is empty for a given field') | ||
return_type = traits.Enum( | ||
'file', 'namedtuple', usedefault=True) |
There was a problem hiding this comment.
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.
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']] |
There was a problem hiding this comment.
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.
index_derivatives = traits.Bool( | ||
False, usedefault=True, | ||
desc='Index derivatives/ sub-directory') | ||
extra_derivatives = traits.List( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra_derivatives = traits.List( | |
extra_derivatives = InputMultiObject( |
(Requires importing ImportMultiObject
along with InputMultiPath
; we should move all *MultiPath
s to *MultiObject
s, but that shouldn't be this PR.)
Well, I merged master and now some other unrelated tests are failing.. Any clues? |
Also, it looks like @jdkent's issue is a more general pybids issue: bids-standard/pybids#343 Also, some derivatives were not being validated because The other issue was deprecated derivatives keys. In the meantime, we can allow some custom keys to be passed in, as you can already do in pybids. I'll look into this. |
Cool, looks like tests pass in Python 2 (aside from those failing due to another issue) |
Yeah, I'm starting to think it's numpy 1.16 that's breaking things somehow. |
Hey, it works! Do you think it's ready to merge @effigies? |
Sorry, I still need to get around to a final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made some comments, one related to a merge conflict I've just bestowed upon you. Let me know what you think.
A little more broadly, I think we're running up against the problem of backwards compatibility, which is going to be a problem, as @jdkent notes, for people attempting to work with derivatives from fmriprep<1.2
.
@jdkent Is it possible that running this interface with bids 0.6.5 gives you the entities you want? If not, is it worth augmenting to support 0.6.5? I'm okay with setting a hard minimum on 0.6.5, so that we have pre-August and post-August BIDS derivatives support without committing to support all the reorganizations that happened before then.
docker/generate_dockerfiles.sh
Outdated
@@ -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" \ |
There was a problem hiding this comment.
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" \
nipype/interfaces/io.py
Outdated
True, usedefault=True, | ||
desc='Generate exception if list is empty for a given field') | ||
return_type = traits.Enum( | ||
'file', 'namedtuple', usedefault=True) |
There was a problem hiding this comment.
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') | ||
extra_derivatives = traits.List( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra_derivatives = traits.List( | |
extra_derivatives = InputMultiObject( |
(Requires importing ImportMultiObject
along with InputMultiPath
; we should move all *MultiPath
s to *MultiObject
s, but that shouldn't be this PR.)
Co-Authored-By: adelavega <aleph4@gmail.com>
Mandatory index_derivatives Co-Authored-By: adelavega <aleph4@gmail.com>
OK, I addressed all changes except What's that about? You're saying I should or shouldn't do it in this PR? |
Regarding backwards compatibility, I think supporting pybids < 0.7 is more trouble than its worth. Turns out its just a single key that is causing problems, so perhaps instead we should allow custom keys to be passed in (which pybids allows), and that can be used to read in older fmripep outputs. This is what @jdkent and I agreed on in Florida. I just wasn't necessarily going to put that into this PR, but I can. |
We should allow multiple paths, but changing all |
Okay. Hopefully nobody else is using this. |
How about if we leave this PR as is (assuming I addressed your others comments and tests pass), and we can address the custom keys argument in another PR? That way @jdkent can test it out (since he has an old fmriprep input he is trying to read in. |
|
Something else unrelated seems to be failing now...
|
Merge master. |
(See #2855.) |
It says I'm already up to date on master. I merged master very recently and that PR was 6 days ago. |
There was one auto test that didnt get added (new file) so hopefully that's the issue. |
Nope, it's #2862. Not sure if that will work. We'll see. |
Code maintenance is super fun! |
1.1.8 (January 28, 2019) * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846) * FIX: Resolve LinAlgError during SVD (nipy#2838) * ENH: Add interfaces wrapping DIPY worflows (nipy#2830) * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737) * ENH: Add FSL `eddy_quad` interface (nipy#2825) * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823) * ENH: Support for BIDS event files (nipy#2845) * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785) * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850) * DOC: Add S3DataGrabber example (nipy#2849) * DOC: Skip conftest module in API generation (nipy#2852) * DOC: Hyperlink DOIs to preferred resolver (nipy#2833) * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862) * MAINT: Drop pytest-xdist requirement, minimum pytest version (nipy#2856) * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855) * tag '1.1.8': (79 commits) MNT: Add @feilong to .zenodo, update ordering MNT: Update .mailmap MNT: Update .zenodo ordering Accept invitation as Zenodo release co-author (see nipy#2864) MAINT: Update .mailmap BF: allowing bids_event_file as alternate input MNT: Update .zenodo ordering MNT: Version 1.1.8 DOC: 1.1.8 changelog Update nipype/interfaces/dipy/tracks.py Update nipype/interfaces/dipy/reconstruction.py MNT: Install numpy!=1.16.0 from conda in Docker Add FSL auto test remake specs Update nipype/interfaces/io.py Remove return type named tuple Update nipype/info.py STY: Whitespace, line length Remove out_ prefix from EddyQuad outputs Apply minor edits from code review ...
This PR updates
BIDSDataGrabber
for pybids 0.7.T1w
vsanat
;bold
vsfunc
)derivatives
and option to add other directories)outfields
from examples, as they are dynamically generated.Need to wait for pybids to officially release 0.7 (and pin version), before merging.