ENH: Enable users to pass JSON filters to select subsets of data #1770

bpinsard · 2019-09-12T18:23:02Z

Changes proposed in this pull request

Adding a --bids_filters option to further specify the options to select input files to the pipeline.
This points to a json files containing a dict with pybids entities filters, that is to be passed to nipype.interfaces.io.BIDSDataGrabber output_query option.
I created the same branch in smriprep with similar changes in run.py and workflow.base.py

While I was coding that I realized that f/smriprep uses in fact niworkflows.interfaces.bids.BIDSDataGrabber (this is a mistaking name btw).
Is there a specific reason to recode that part, could it be possible to use the original nipype one, or it is missing a feature?
I need you insight there.
Thanks

Documentation that should be reviewed

TODO/WIP

welcome · 2019-09-12T18:23:03Z

Thanks for opening this pull request! We have detected this is the first time for you to contribute to fMRIPrep. Please check out our contributing guidelines.
We invite you to list yourself as a fMRIPrep contributor, so if your name is not already mentioned, please modify the .zenodo.json file with your data right above Russ' entry. Example:

{
   "name": "Contributor, New FMRIPrep",
   "affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>"
},
{
   "name": "Poldrack, Russell A.",
   "affiliation": "Department of Psychology, Stanford University",
   "orcid": "0000-0001-6755-0259"
},

Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.

bpinsard · 2019-09-12T19:26:48Z

I made corresponding changes in niworkflows: https://github.com/bpinsard/niworkflows/tree/enh/bids_filter
and smriprep: https://github.com/bpinsard/smriprep/tree/enh/bids_filter

effigies · 2019-09-12T19:35:57Z

Do you have an example JSON filter?

bpinsard · 2019-09-12T19:37:54Z

For instance, the following add a filter for reconstruction (rec-xxx), and bold is the same filter as default.

{
    "t1w": {
        "datatype": "anat",
        "reconstruction": null,
        "suffix": "T1w"
    },
    "bold": {
        "datatype": "func",
        "suffix": "bold"
    }
}

( adding this for reference: https://github.com/bids-standard/pybids/blob/master/bids/layout/config/bids.json )

oesteban

Minimal nitpick 👍 . Will check the PR to niworkflows when ready.

fmriprep/cli/run.py

Co-Authored-By: Oscar Esteban <code@oscaresteban.es>

fmriprep/cli/run.py

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

pep8speaks · 2019-10-23T16:23:16Z

Hello @bpinsard! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-14 13:56:28 UTC

hjjh

emdupre · 2019-12-11T14:57:30Z

I wonder if this work wouldn't make sense to upstream to BIDS Apps more generally ? I'm thinking in particular of the spec @effigies was co-leading. It seems like a general question of how BIDS Apps should handle multiple eligible inputs.

@bpinsard and I discussed this in person, and I would have thought the best "BIDS" way to do this for the moment is just to modify a study's .bidsignore when generating the derivatives. The work here is a much more rigorous solution, but it would seem like something that should exist beyond f/smriprep and/or niworkflows ?

bpinsard · 2019-12-11T15:08:11Z

@emdupre
This is definitely a recurring issues when using BIDSApps on complex datasets. It would make sense to factor it, but for now BIDS grabbing is coded in each run.py of each BIDSApps.
For fMRIPrep, as the BIDS grabbing is not at the top level, but goes through multiple layers of fmriprep/smriprep/niworkflows, this is more difficult to factor it with other BIDSApps.

emdupre · 2019-12-11T21:04:06Z

For fMRIPrep, as the BIDS grabbing is not at the top level, but goes through multiple layers of fmriprep/smriprep/niworkflows, this is more difficult to factor it with other BIDSApps.

I completely understand that 😸

I just mean that this seems like something that we would want to upstream to the BIDS Execution specification, since it strikes me as a very general issue.

EDIT: And sorry if this is the wrong place to post this ! There are a few cross-linked issues and PRs, now, so I'm not sure where's the best place to have this discussion....

bpinsard · 2019-12-13T16:09:35Z

Tests fails due to dependency on niworkflow changes, same for PR on smriprep.
Otherwise I think these changes are ready.

effigies · 2020-02-12T02:38:00Z

I think this is in good shape. How do we feel about squeezing it in to 20.0.0? It adds a CLI option but does not change anything when the option is not passed.

Need to merge nipreps/smriprep#143 and release niworkflows and smriprep, but we can do that tomorrow if everybody's happy.

@bpinsard Have you been actually testing this? I can try it with a multi-session OpenNeuro dataset tomorrow if not. Or we can push a docker/ branch to get a test image up on Docker Hub.

mgxd

Minor suggestion, but overall this looks good. We can definitely get this in for 20.0.0, will target a smriprep / niworkflow release for tomorrow.

mgxd · 2020-02-13T22:23:15Z

fmriprep/cli/run.py

@@ -76,6 +77,11 @@ def get_parser():
    # Re-enable when option is actually implemented
    # g_bids.add_argument('-r', '--run-id', action='store', default='single_run',
    #                     help='select a specific run to be processed')
+    g_bids.add_argument(
+        '--bids-filters', action='store', type=Path,


I think making it explicit this is a file will improve clarity and be more in line with the other file cli arguments

Suggested change

'--bids-filters', action='store', type=Path,

'--bids-filter-file', action='store', type=Path,

mgxd · 2020-02-13T22:23:43Z

fmriprep/cli/run.py

@@ -499,6 +505,8 @@ def build_workflow(opts, retval):
    bids_dir = opts.bids_dir.resolve()
    output_dir = opts.output_dir.resolve()
    work_dir = opts.work_dir.resolve()
+    bids_filters = json.loads(opts.bids_filters.read_text()) \


Suggested change

bids_filters = json.loads(opts.bids_filters.read_text()) \

bids_filters = json.loads(opts.bids_filter_file.read_text()) \

mgxd · 2020-02-13T22:23:57Z

fmriprep/cli/run.py

@@ -499,6 +505,8 @@ def build_workflow(opts, retval):
    bids_dir = opts.bids_dir.resolve()
    output_dir = opts.output_dir.resolve()
    work_dir = opts.work_dir.resolve()
+    bids_filters = json.loads(opts.bids_filters.read_text()) \
+        if opts.bids_filters else None


Suggested change

if opts.bids_filters else None

if opts.bids_filter_file else None

mgxd

Cool - thanks. I suggested the updated pins, will commit once those are live on pypi and then we can merge this in

setup.cfg

first quick attempt to add bids filters to further specify input

ec67041

bpinsard added 2 commits September 12, 2019 14:24

fix json loading

6ed1e21

make the changes to use niworkflows BIDSDataGrabber

98f498f

oesteban reviewed Sep 12, 2019

View reviewed changes

fmriprep/cli/run.py Outdated Show resolved Hide resolved

fmriprep/cli/run.py Outdated Show resolved Hide resolved

bpinsard and others added 2 commits September 30, 2019 09:56

Update fmriprep/cli/run.py

a8ddc78

Co-Authored-By: Oscar Esteban <code@oscaresteban.es>

Update fmriprep/cli/run.py

6658403

Co-Authored-By: Oscar Esteban <code@oscaresteban.es>

effigies reviewed Sep 30, 2019

View reviewed changes

fmriprep/cli/run.py Outdated Show resolved Hide resolved

bpinsard and others added 3 commits September 30, 2019 10:03

hyphen-separated option

67afdcc

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

fix formatting and tests

d622879

Merge remote-tracking branch 'upstream/master' into enh/bids_filter

a6252de

bpinsard mentioned this pull request Sep 30, 2019

ENH: Enable optional BIDS entity filtering to data collection utility nipreps/niworkflows#407

Merged

oesteban force-pushed the master branch from 0a3fe45 to b3a4d24 Compare October 9, 2019 19:25

Merge branch 'master' into enh/bids_filter

a880a8d

bpinsard added 3 commits October 23, 2019 17:19

Merge pull request #1 from poldracklab/master

40b0b05

hjjh

Merge branch 'master' into enh/bids_filter

e9625a2

pep8+merge

42792ea

bpinsard mentioned this pull request Nov 27, 2019

ENH: Enable users to pass JSON filters to select subsets of data nipreps/smriprep#143

Merged

bpinsard added 2 commits December 10, 2019 14:31

docs, fixes

f589334

merge

55cfb03

fix testing

b12ffb8

oesteban mentioned this pull request Jan 28, 2020

Suggested: ignore anatomical data from sessions without functional sessions #1925

Closed

effigies added 2 commits February 11, 2020 16:36

Merge remote-tracking branch 'upstream/master' into enh/bids_filter

8976cc6

PIN: niworkflows master, smriprep feature branch

7d0a9f1

effigies changed the title ~~first quick attempt to add bids filters to further specify pipeline inputs~~ ENH: Enable users to pass JSON filters to select subsets of data Feb 12, 2020

effigies requested review from oesteban and mgxd February 12, 2020 17:45

mgxd reviewed Feb 13, 2020

View reviewed changes

effigies added 2 commits February 13, 2020 21:08

ENH: Change CLI option name

f2de5c6

Merge remote-tracking branch 'upstream/master' into enh/bids_filter

6d86c0b

effigies requested a review from mgxd February 14, 2020 02:10

mgxd approved these changes Feb 14, 2020

View reviewed changes

setup.cfg Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

PIN: smriprep pre-release

ec73b2d

effigies merged commit f682c9e into nipreps:master Feb 14, 2020

utooley mentioned this pull request Mar 10, 2020

DOC: BIDS filter input example #2028

Merged

effigies mentioned this pull request May 27, 2020

Retain session information for data that does not combine multiple sessions nipreps/smriprep#197

Closed

effigies mentioned this pull request Oct 5, 2023

Feature request: filter input dataset by other ids nipreps/mriqc#760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Enable users to pass JSON filters to select subsets of data #1770

ENH: Enable users to pass JSON filters to select subsets of data #1770

bpinsard commented Sep 12, 2019 •

edited

Loading

welcome bot commented Sep 12, 2019

bpinsard commented Sep 12, 2019

effigies commented Sep 12, 2019

bpinsard commented Sep 12, 2019 •

edited

Loading

oesteban left a comment

pep8speaks commented Oct 23, 2019 •

edited

Loading

emdupre commented Dec 11, 2019

bpinsard commented Dec 11, 2019

emdupre commented Dec 11, 2019 •

edited

Loading

bpinsard commented Dec 13, 2019

effigies commented Feb 12, 2020

mgxd left a comment

mgxd Feb 13, 2020

mgxd Feb 13, 2020

mgxd Feb 13, 2020

mgxd left a comment

	'--bids-filters', action='store', type=Path,
	'--bids-filter-file', action='store', type=Path,

	bids_filters = json.loads(opts.bids_filters.read_text()) \
	bids_filters = json.loads(opts.bids_filter_file.read_text()) \

	if opts.bids_filters else None
	if opts.bids_filter_file else None

ENH: Enable users to pass JSON filters to select subsets of data #1770

ENH: Enable users to pass JSON filters to select subsets of data #1770

Conversation

bpinsard commented Sep 12, 2019 • edited Loading

Changes proposed in this pull request

Documentation that should be reviewed

welcome bot commented Sep 12, 2019

bpinsard commented Sep 12, 2019

effigies commented Sep 12, 2019

bpinsard commented Sep 12, 2019 • edited Loading

oesteban left a comment

Choose a reason for hiding this comment

pep8speaks commented Oct 23, 2019 • edited Loading

Comment last updated at 2020-02-14 13:56:28 UTC

emdupre commented Dec 11, 2019

bpinsard commented Dec 11, 2019

emdupre commented Dec 11, 2019 • edited Loading

bpinsard commented Dec 13, 2019

effigies commented Feb 12, 2020

mgxd left a comment

Choose a reason for hiding this comment

mgxd Feb 13, 2020

Choose a reason for hiding this comment

mgxd Feb 13, 2020

Choose a reason for hiding this comment

mgxd Feb 13, 2020

Choose a reason for hiding this comment

mgxd left a comment

Choose a reason for hiding this comment

bpinsard commented Sep 12, 2019 •

edited

Loading

bpinsard commented Sep 12, 2019 •

edited

Loading

pep8speaks commented Oct 23, 2019 •

edited

Loading

emdupre commented Dec 11, 2019 •

edited

Loading