DOC: BIDS filter input example #2028

utooley · 2020-03-10T19:15:16Z

#2027, one stab at it.

Added a few lines to the Usage Notes as an example of input to --bids-filter-file. Took me a while of digging through pybids to figure out how to set the config.json up properly; I think that it would be useful to link to an example like that in the discussion of #1770, but couldn't figure out where to put it.

Documentation that should be reviewed

Usage Notes - Positional arguments - bids-filter-file

auto-comment · 2020-03-10T19:15:19Z

Thank your for raising your pull request.

Some of the fMRIPRep maintainers will review your changes as soon as time permits.
I'm attaching below a Review Checklist for the reviewers, to check off during the
review.

PR Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

As the reviewer I confirm that there are no conflicts of interest for me to review this work.

Please check what applies in the following aspects of the PR:

Code documentation

New functions and modules are documented following the guidelines.
Existing code required amendments in documentation and has been updated.
Sufficient inline comments ensure readability by other contributors in the long term.

Documentation site

The modifications to fMRIPrep in this PR do not require extra documentation. This is a common situation when a bug fix that does not change the code base substantially is submitted.
This PR requires documentation. The corresponding documentation will be submitted as an additional PR in the future.
This PR requires extra documentation, and will be included within this PR. Please check the boxes that apply best:
- An existing feature is substantially modified, changing the interface and/or logic of a module.
- A new feature is being added, therefore full documentation of it will be included
- This PR addresses an error in existing documentation.
Yes, all expected sections of documentation were generated by the CI build, and look as expected

Tests

The new functionalities in this PR are covered by the existing tests
New test batteries have been included with this PR

Data

This PR does not require any additional data to be uploaded to OSF.
This PR requires additional data for tests.

Dependencies: smriprep

This PR does not require any update on smriprep; or
smriprep has correctly been pinned.

Dependencies: niworkflows

This PR does not require any update on niworkflows; or
niworkflows has correctly been pinned.

Dependencies: sdcflows

This PR does not require any update on sdcflows; or
sdcflows has correctly been pinned.

Dependencies: Nipype

This PR does not require any update on nipype; or
nipype has correctly been pinned.

Dependencies: other

This PR does not require any update on other dependencies; or
other dependencies have correctly been pinned.

Reports generated within CI tests

Yes, I have checked the reports of ds005 and they are not modified or the changes are as expected
Yes, I have checked the reports of ds054 and they are not modified or the changes are as expected
Yes, I have checked the reports of ds010 and they are not modified or the changes are as expected

oesteban · 2020-03-11T02:06:11Z

Could you rebase with upstream/master? The latest PR merge should fix the issues you are encountering (namely, dropping one of the flake8 extensions).

utooley · 2020-03-11T14:33:48Z

Sorry, I don't think I did this properly...I was having trouble with rebase, possibly because of a bug in OSX that kept me in an infinite loop, possibly because I haven't ever had to use it before and was trying to learn by doing. Please tell me if I should fix something!

Edit: Building docs locally I ran into issues with fmriprep.cli.parser._build_parser not being found, but the artifact here looks fine.

effigies · 2020-03-11T14:45:14Z

Can you show git remote -v? It will make it easier to give rebase instructions.

utooley · 2020-03-11T14:56:40Z

origin	git@github.com:utooley/fmriprep.git (fetch)
origin	git@github.com:utooley/fmriprep.git (push)
upstream	https://github.com/poldracklab/fmriprep.git (fetch)
upstream	https://github.com/poldracklab/fmriprep.git (push)```

I did :

git fetch upstream
git rebase upstream/master

Got caught in a merge conflict, resolved it, but then an infinite loop of "no changes" when trying to git rebase --continue. Apparently there's this which I've now fixed, but not sure if that was really the problem.

effigies · 2020-03-11T15:04:48Z

Looks like there's a merge commit. (See network.)

Just git rebase -i upstream/master and set the plan to:

pick ba0af72

Save and quit, and it should go smoothly.

utooley · 2020-03-11T16:31:06Z

Alright, I think I fixed it--let me know if this is wrong! Took me a bit to figure out that I needed to force-push.

effigies · 2020-03-11T17:09:07Z

Took me a bit to figure out that I needed to force-push.

Ah, sorry. Yes. I forgot about that. This looks like a good rebase.

mgxd

Thanks! Dictionaries are hard to prettify with argparse, I think this would be cleaner as a section of the documentation (perhaps under usage.rst or faq.rst)

Then you could just add a pointer to it similar to how --output-spaces does:
https://github.com/poldracklab/fmriprep/blob/1d93d9e5599d08347ba2b85a4d4590a4c9273814/fmriprep/cli/parser.py#L156

pep8speaks · 2020-03-16T20:27:42Z

Hello @utooley! 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-03-17 20:15:56 UTC

utooley · 2020-03-16T20:38:07Z

Alright--I added a section to the FAQ, and a pointer in parser.py, though PEP8 doesn't seem to link how long the link is (I tried to break it into a new line in b4eb01b but it didn't seem to work properly?). Not sure what conventions for that are.

mgxd

Thanks, looks good! Hopefully, that suggestion will fix the style error

fmriprep/cli/parser.py

utooley · 2020-03-17T16:12:42Z

Thanks for the tip! I didn't think of formatting that way.

mgxd

super tiny nitpick but otherwise this is all set, thank you!

docs/faq.rst

Co-Authored-By: Mathias Goncalves <goncalves.mathias@gmail.com>

effigies

This looks good, but I think we can make it a bit more helpful by making clear which queries can be overridden and what exactly is being overridden.

Also some style suggestions.

docs/faq.rst

fmriprep/cli/run.py

For format and clarity on queries. Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies

Thanks, and sorry for all the back-and-forth. This LGTM.

* CLI changes from #2028 were ported to `parser.py` 20.0.4 (March 17, 2020) ======================= A bug-fix release improving documentation for filtering BIDS files and standardizing CIFTI volume orientation. With thanks to Ursula Tooley for the contribution. * DOC: FAQ section for BIDS filter (#2028) * FIX: Ensure BOLD and label orientations are equal (`nipreps/niworkflows#477`_). .. _`nipreps/niworkflows#477`: nipreps/niworkflows#477

utooley force-pushed the docs/bids_filter_input branch from cbe90e6 to d9a4dd6 Compare March 11, 2020 16:22

mgxd reviewed Mar 13, 2020

View reviewed changes

utooley force-pushed the docs/bids_filter_input branch from d9a4dd6 to d9bdacb Compare March 16, 2020 18:51

mgxd approved these changes Mar 17, 2020

View reviewed changes

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

mgxd reviewed Mar 17, 2020

View reviewed changes

docs/faq.rst Outdated Show resolved Hide resolved

mgxd changed the base branch from master to maint/20.0.x March 17, 2020 17:26

oesteban force-pushed the docs/bids_filter_input branch from 7dcf820 to 069ed21 Compare March 17, 2020 18:25

mgxd force-pushed the docs/bids_filter_input branch from 069ed21 to 71d19f7 Compare March 17, 2020 18:42

utooley and others added 5 commits March 17, 2020 14:50

add section to faq and pointer in parser.py

177a84a

pep8 and formatting fixes

c832e60

fixed FAQ format that stopped tests

33f7ae7

Update docs/faq.rst

3a40d97

Co-Authored-By: Mathias Goncalves <goncalves.mathias@gmail.com>

DOC: pointer to filtering FAQ

1d718a1

mgxd force-pushed the docs/bids_filter_input branch from 71d19f7 to 1d718a1 Compare March 17, 2020 18:56

effigies reviewed Mar 17, 2020

View reviewed changes

docs/faq.rst Outdated Show resolved Hide resolved

docs/faq.rst Outdated Show resolved Hide resolved

docs/faq.rst Outdated Show resolved Hide resolved

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

Apply suggestions from code review

82206ed

For format and clarity on queries. Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies approved these changes Mar 17, 2020

View reviewed changes

mgxd merged commit 2f631f8 into nipreps:maint/20.0.x Mar 17, 2020

mgxd added this to the 20.0.4 milestone Mar 17, 2020

utooley deleted the docs/bids_filter_input branch April 6, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: BIDS filter input example #2028

DOC: BIDS filter input example #2028

utooley commented Mar 10, 2020

auto-comment bot commented Mar 10, 2020

oesteban commented Mar 11, 2020

utooley commented Mar 11, 2020 •

edited

effigies commented Mar 11, 2020

utooley commented Mar 11, 2020 •

edited

effigies commented Mar 11, 2020

utooley commented Mar 11, 2020

effigies commented Mar 11, 2020

mgxd left a comment

pep8speaks commented Mar 16, 2020 •

edited

utooley commented Mar 16, 2020

mgxd left a comment

utooley commented Mar 17, 2020

mgxd left a comment

effigies left a comment

effigies left a comment

DOC: BIDS filter input example #2028

DOC: BIDS filter input example #2028

Conversation

utooley commented Mar 10, 2020

Documentation that should be reviewed

auto-comment bot commented Mar 10, 2020

PR Review

Code documentation

Documentation site

Tests

Data

Dependencies: smriprep

Dependencies: niworkflows

Dependencies: sdcflows

Dependencies: Nipype

Dependencies: other

Reports generated within CI tests

oesteban commented Mar 11, 2020

utooley commented Mar 11, 2020 • edited

effigies commented Mar 11, 2020

utooley commented Mar 11, 2020 • edited

effigies commented Mar 11, 2020

utooley commented Mar 11, 2020

effigies commented Mar 11, 2020

mgxd left a comment

Choose a reason for hiding this comment

pep8speaks commented Mar 16, 2020 • edited

Comment last updated at 2020-03-17 20:15:56 UTC

utooley commented Mar 16, 2020

mgxd left a comment

Choose a reason for hiding this comment

utooley commented Mar 17, 2020

mgxd left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

utooley commented Mar 11, 2020 •

edited

utooley commented Mar 11, 2020 •

edited

pep8speaks commented Mar 16, 2020 •

edited