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] fmriprep-docker documentation #1515

Merged
merged 6 commits into from
Feb 25, 2019
Merged

Conversation

franklin-feingold
Copy link
Contributor

Changes proposed in this pull request

This PR will address #1488 and #1129. This PR provides additional documentation to fmriprep-docker

Documentation that should be reviewed

Provided documentation to fmriprep-docker

Added documentation to fmriprep-docker call
local version of this file deleted spaces at the end of certain sentences, added them back in
@welcome
Copy link

welcome bot commented Feb 22, 2019

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.

@franklin-feingold
Copy link
Contributor Author

I'm not sure why the ds tests failed, I am only changing the documentation

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

left some suggestions and comments 👍 . Thanks a lot for this!

@@ -26,20 +26,33 @@ In order to run fmriprep in a Docker container, Docker must be `installed
<https://docs.docker.com/engine/installation/>`_.
Once Docker is installed, the recommended way to run fmriprep is to use the
fmriprep-docker_ wrapper, which requires Python and an Internet connection.
``fmriprep-docker`` is a streamlined command to run fmriprep without having to
properly mount directories (``fmriprep-docker`` does this for you)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
properly mount directories (``fmriprep-docker`` does this for you)
properly mount directories (``fmriprep-docker`` does this for you).

Copy link
Member

Choose a reason for hiding this comment

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

Just to rewrite the whole sentence:

``fmriprep-docker`` is a command that allows you to write your command line
as if you were running ``fmriprep`` directly, and converts it into a ``docker``
command.


To install::

$ pip install --user --upgrade fmriprep-docker

When run, ``fmriprep-docker`` will generate a Docker command line for you,
print it out for reporting purposes, and then run the command, e.g.::
When you run, ``fmriprep-docker`` it will generate a Docker command line for you,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When you run, ``fmriprep-docker`` it will generate a Docker command line for you,
When you run ``fmriprep-docker``, it will generate a Docker command line for you,

When run, ``fmriprep-docker`` will generate a Docker command line for you,
print it out for reporting purposes, and then run the command, e.g.::
When you run, ``fmriprep-docker`` it will generate a Docker command line for you,
print it out for reporting purposes, and then run the command without further action
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print it out for reporting purposes, and then run the command without further action
print it out for reporting purposes, and then execute it without further action


$ fmriprep-docker /path/to/data/dir /path/to/output/dir participant
RUNNING: docker run --rm -it -v /path/to/data/dir:/data:ro \
-v /path/to_output/dir:/out poldracklab/fmriprep:1.0.0 \
/data /out participant
...

For ``fmriprep-docker`` all the options you would typically pass to fmriprep,
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this paragraph. I think it should differentiate between docker arguments that you can pass in (e.g. environment variables and mounting dirs) and the arguments to fmriprep.

Copy link
Member

Choose a reason for hiding this comment

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

@oesteban Not sure exactly what you're suggesting here. As in we should add a section for additional fmriprep-docker-specific options? For that I would direct readers to The Docker Wrapper CLI, to avoid cluttering the basic "How to run fMRIPrep" page with those details.


We have published a `step-by-step tutorial
<http://reproducibility.stanford.edu/fmriprep-tutorial-running-the-docker-image/>`_
illustrating how to run ``fmriprep-docker``. This tutorial also provides valuable
Copy link
Member

Choose a reason for hiding this comment

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

Start sentence in a new line, down the line that is easier to maintain.

@@ -26,20 +26,33 @@ In order to run fmriprep in a Docker container, Docker must be `installed
<https://docs.docker.com/engine/installation/>`_.
Once Docker is installed, the recommended way to run fmriprep is to use the
fmriprep-docker_ wrapper, which requires Python and an Internet connection.
``fmriprep-docker`` is a streamlined command to run fmriprep without having to
properly mount directories (``fmriprep-docker`` does this for you)
Copy link
Member

Choose a reason for hiding this comment

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

Just to rewrite the whole sentence:

``fmriprep-docker`` is a command that allows you to write your command line
as if you were running ``fmriprep`` directly, and converts it into a ``docker``
command.


$ fmriprep-docker /path/to/data/dir /path/to/output/dir participant
RUNNING: docker run --rm -it -v /path/to/data/dir:/data:ro \
-v /path/to_output/dir:/out poldracklab/fmriprep:1.0.0 \
/data /out participant
...

For ``fmriprep-docker`` all the options you would typically pass to fmriprep,
you can use for ``fmriprep-docker``. ``fmriprep-docker`` is mostly mounting
the paths and environmental variables.
Copy link
Member

Choose a reason for hiding this comment

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

An alternative paragraph:

``fmriprep-docker`` accepts all of the typical options for ``fmriprep``,
automatically translating directories into Docker mount points.


$ fmriprep-docker /path/to/data/dir /path/to/output/dir participant
RUNNING: docker run --rm -it -v /path/to/data/dir:/data:ro \
-v /path/to_output/dir:/out poldracklab/fmriprep:1.0.0 \
/data /out participant
...

For ``fmriprep-docker`` all the options you would typically pass to fmriprep,
Copy link
Member

Choose a reason for hiding this comment

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

@oesteban Not sure exactly what you're suggesting here. As in we should add a section for additional fmriprep-docker-specific options? For that I would direct readers to The Docker Wrapper CLI, to avoid cluttering the basic "How to run fMRIPrep" page with those details.

@effigies
Copy link
Member

Oh, looks like I reviewed at exactly the wrong moment... Let me know if you want me to go through again.

@franklin-feingold
Copy link
Contributor Author

It was at the same time! I'll apply your edits right now and then ping you for your review

applying suggestions and recommendations from Chris M
@franklin-feingold
Copy link
Contributor Author

@effigies - recent commit should have applied both Oscar's and your's comments and suggestions

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

A couple minor adjustments.

@@ -26,20 +26,34 @@ In order to run fmriprep in a Docker container, Docker must be `installed
<https://docs.docker.com/engine/installation/>`_.
Once Docker is installed, the recommended way to run fmriprep is to use the
fmriprep-docker_ wrapper, which requires Python and an Internet connection.
``fmriprep-docker`` is a command that allows you to write your command line
as if you were running ``fmriprep`` directly, converts it into a ``docker``
Copy link
Member

Choose a reason for hiding this comment

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

"and converts" or "converting"

This tutorial also provides valuable troubleshooting insights and advice on
what to do after fmriprep has run.


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@franklin-feingold
Copy link
Contributor Author

adjustments applied - thank you for reviewing!

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