FIX: Various BIDSDataGrabber fixes

[effigies]

Summary

In future pybids versions, derivatives will not be automatically indexed, and test data has been added to help ensure that derivatives are properly handled.

This is a potentially breaking change, so we should consider if we would prefer some other approach.

Fixes #2650.

List of changes proposed in this PR (pull-request)

• Add strict input to BIDSDataGrabber that currently excludes derivatives/, code/ and sourcedata/ subdirectories from indexing.
• Only return images by default, per documentation.
• Make test more resilient by checking whether expected file is present, not first.

Acknowledgment

• (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

[effigies]

cc @adelavega

[mgxd]

This should be more elastic, I'd prefer adding an option to the inputspec:

exclude = traits.List(traits.Str, value=['derivatives/', 'code/', 'sourcedata/'], usedefault=True, ...)

and then plugging that in here

[adelavega]

Agreed. There are many instances in which you would want to index derivatives. By the way, if that's the case you probably also want to initate the BIDSLayout with 'bids' and 'derivatives' spec for the derivatives folder... So maybe we need to re-think how input folders are specified in this module (sort of how fitlins does it?)

[effigies]

There are weird compatibility issues here, such as whether the interface should have consistent behavior across some pretty wild API changes (some completed, some planned) in pybids, or just to call whatever version is installed, and have users pray.

For example, I don't think exclude was present in pybids when we added this interface. And once we stop indexing derivatives/ by default, the appropriate way to modulate this will be to add derivatives, instead. So at that point, do we change the default exclude values, the effect of which will vary based on pybids version, or what?

[mgxd]

it looks like BIDSLayout added kwarg support in version 0.0.2, and I doubt anyone is using an older version than that - I don't think it'll be breaking to add exclude, at worst it will just be ignored.

However, if pybids will be setting this automatically in the future, perhaps we shouldn't set a default in nipype. To fix the tests, we can just make them exclude the necessary directories for now.

[adelavega]

Yeah, fair enough. Maybe for now we can just add the exclude trait (which should be fairly stable), and not worry about indexing derivatives for now. I image 99% of users only want to index core BIDS files. Then we can wait for pybids to mature a bit (tal said he might add a 'index_derivatives' argument).

[effigies]

Is the goal to have this provide all options that BIDSLayout provides? That seems dangerous, given how much of a moving target that is. I think more we should consider BIDSDataGrabber its own API and strive to make it behave consistently across pybids versions.

That's one reason I'm very hesitant to add exclude, because the interpretation of it changes based on what the default inclusions/exclusions are.

[adelavega]

I think the original goal was that it was a BIDSLayout wrapper, but I don't think it needs to mirror all the functionality. I'm fine leaving that off for now and reconsidering later once pybids has matured/stabilized a bit (or at least until BIDS Derivatives becomes more stable itself). I do think we're going to want to deal w/ how derivatives are indexed sooner or later (otherwise it limits the utility of BIDSDataGrabber quite a bit IMO).

[effigies]

Cool. I also think there's potential for multiple interfaces, if we get to the point where we have to keep cramming options into the one. So it's worth thinking about what are the essential pieces of functionality we want, and at what point is the complexity cost of more options greater than a new interface with a different target.

This is part of why I'm writing my own interfaces in FitLins... once I see what all I need, I can think about whether merging with BIDSDataGrabber makes sense or not.

[adelavega]

See my comments on the issue, but I think the other part of the problem is that event files should not be indexed by the default behavior, only bold and anat files. So you need to add type='bold' to the default query.

[codecov-io]

Codecov Report

Merging #2651 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #2651      +/-   ##
==========================================
+ Coverage   67.63%   67.63%   +<.01%     
==========================================
  Files         340      340              
  Lines       43030    43034       +4     
  Branches     5325     5326       +1     
==========================================
+ Hits        29102    29108       +6     
+ Misses      13226    13224       -2     
  Partials      702      702

Flag	Coverage Δ
#smoketests	50.51% <16.66%> (-0.01%)	⬇️
#unittests	65.1% <66.66%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/io.py	54.41% <66.66%> (-0.17%)	⬇️
nipype/utils/draw_gantt_chart.py	12.82% <0%> (-1.93%)	⬇️
nipype/pipeline/plugins/legacymultiproc.py	61.3% <0%> (+5.35%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04cfa7a...799aacf. Read the comment docs.

[adelavega]

New changes look good to me. You could have instead put type : bold but what you did also works.