Skip to content

FIX: Various BIDSDataGrabber fixes #2651

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.

Sign up for GitHub

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 5 commits into from
Jul 25, 2018
Merged

FIX: Various BIDSDataGrabber fixes #2651

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1efa6cd
ENH: Exclude non-"proper" BIDS subdirectories
effigies Jul 25, 2018
db3b0ed
ENH: Add strict mode to BIDSDataGrabber
effigies Jul 25, 2018
4ab3437
FIX: BIDSDataGrabber indexing non-images
effigies Jul 25, 2018
b0b6670
TEST: More resilient tests
effigies Jul 25, 2018
799aacf
FIX: Dict syntax
effigies Jul 25, 2018
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
3 changes: 2 additions & 1 deletion nipype/interfaces/io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2828,7 +2828,8 @@ def _run_interface(self, runtime):
return runtime

def _list_outputs(self):
layout = gb.BIDSLayout(self.inputs.base_dir)
layout = gb.BIDSLayout(self.inputs.base_dir,
exclude=['derivatives/', 'code/', 'sourcedata/'])
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 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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


# If infield is not given nm input value, silently ignore
filters = {}
Expand Down