Skip to content
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

DOC: Altered CLI option grouping #2944

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

Lestropie
Copy link
Contributor

Back when I commenced some work on #2207 (from which I'll need to familiarise myself with #2913), I started by looking at the CLI and thinking about what the right interface would be for such a capability (see eg. #2805, #2806). I immediately found some command-line options that really felt as though they were in the wrong place. So here I've separated out the set of CLI changes that I'd generated in the course of that little experiment.

The git diff is quite unclean, so I've produced the set of options that would be moved and their corresponding old and new option groups.

Option Old option group New option group
--anat-only Options to handle performance Options for performing only a subset of the workflow
--boilerplate-only Options to handle performance Options for performing only a subset of the workflow
--md-only-boilerplate Options to handle performance Options for modulating outputs
--cifti-output Surface preprocessing options Workflow configuration
--error-on-aroma-warnings Options to handle performance Specific options for running ICA_AROMA
--verbose Options to handle performance Other options
--me-output-echos Workflow configuration Options for modulating outputs
--medial-surface-nan Workflow configuration Options for modulating outputs
--no-submm-recon Surface preprocessing options Specific options for FreeSurfer processing
--output-layout Other options Options for modulating outputs
--sloppy Other options Options to handle performance
--track-carbon Other options Options for carbon tracking
--country-code Other options Options for carbon tracking

New option groups:

  • "Options for performing only a subset of the workflow"
  • "Options for modulating outputs"
  • "Options for carbon tracking"

Deleted option groups:

  • "Surface preprocessing options"

In particular, "Options for performing only a subset of the workflow" is what I was looking for in the context of #2207, where I had initially added command-line options "--fmri_withhold" and "--fmri_regenerate" (the naming of such options should perhaps be discussed in #2913).

Open to modification of proposed changes, or indeed addition of other changes (since this would be a good opportunity to review placement of the complete set of command-line options). For instance I'm currently looking at [ --output-spaces, --cifti-output, --me-output-echos ] as a candidate option group.

@welcome
Copy link

welcome bot commented Jan 31, 2023

Thanks for opening this pull request! It looks like this is your first time contributing to fMRIPrep. 😄
We invite you to list yourself as an fMRIPrep contributor. To learn more about what that entails and how we credit our contributors, please check out the contributing guidelines. If your name is not already on the list, please insert it, in alphabetical order, into the contributors.json file. Example:

   "name": "Contributor, New FMRIPrep",
   "affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>"
}, ```

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.

Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @Lestropie! I agree - our CLI grouping has gotten rather large and disorganized, so a restructuring is much needed. I've had a quick look and the changes look sensible to me, just left a few comments

If you would like, feel free to add yourself to the contributors file

fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
fmriprep/cli/parser.py Outdated Show resolved Hide resolved
Lestropie and others added 2 commits February 1, 2023 11:47
Fixes erroneous removal of capitalisation and re-introduction of deprecated --topup-max-vols option introduced in 22dfe4b, that arose as a result of manual merging of modifications made against an older version of the code.

Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
@Lestropie
Copy link
Contributor Author

Good catches @mgxd; should all be resolved.

@effigies effigies marked this pull request as ready for review February 2, 2023 15:51
@effigies effigies merged commit fc22100 into nipreps:master Feb 2, 2023
@effigies effigies added this to the 23.0.0 milestone Apr 20, 2023
@Lestropie Lestropie deleted the parser_option_grouping branch April 25, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants