-
Notifications
You must be signed in to change notification settings - Fork 294
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
Updated standalone installation requirements #2009
Conversation
Thanks for opening this pull request! It looks like this is your first time contributing to fMRIPrep. 😄 "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. |
Thank your for raising your pull request. Some of the fMRIPRep maintainers will review your changes as soon as time permits. PR ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
Please check what applies in the following aspects of the PR: Code documentation
Documentation site
Tests
Data
Dependencies: smriprep
Dependencies: niworkflows
Dependencies: sdcflows
Dependencies: Nipype
Dependencies: other
Reports generated within CI tests
|
docs/installation.rst
Outdated
- FreeSurfer_ (version 6.0.1) | ||
- `ICA-AROMA <https://github.com/rhr-pruim/ICA-AROMA/>`_ (version 0.4.1-beta) | ||
- `bids-validator <https://github.com/bids-standard/bids-validator>`_ (version 1.1.0) | ||
- `ICA-AROMA <https://github.com/oesteban/ICA-AROMA/archive/v0.4.5.tar.gz>`_ (version 0.4.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. I didn't realize we have an unofficial release in here.
@oesteban I see @maartenmennes merged maartenmennes/ICA-AROMA#43. Perhaps we should pin:
- `ICA-AROMA <https://github.com/oesteban/ICA-AROMA/archive/v0.4.5.tar.gz>`_ (version 0.4.5) | |
- `ICA-AROMA <https://github.com/maartenmennes/ICA-AROMA/archive/e8d7a58.tar.gz>`_ (commit e8d7a58, post v0.4.4-beta) |
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my reading of the Dockerfile was not in fact as careful as I thought it was. C3D is installed when other packages are pulled from Neurodebian (as convert3d). So the line:
C3D <https://sourceforge.net/projects/c3d/>
_ (version 1.0.0)
Should NOT be removed from the standalone dependencies. Should I open a new PR, or can this one be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one can be fixed. Just push to the same branch and it'll update.
Merge notes: * Fix gh-2014 appears to have been made unnecessary by gh-2018. Tag message: 20.0.2 (March 6, 2020) ====================== A bug squashing release in the 20.0.x series. This release fixes the use of custom templates within the docker wrapper, remedies crashes when FreeSurfer HOME was not set, and improves the documentation for local installations. With thanks to Blaise Frederick for the contribution. * DOC: Update standalone installation requirements (#2009) * FIX: Crashes whenever FREESURFER_HOME is not set (#2014) * FIX: Local template mounting (wrapper) (#2020) * MAINT: Pin minor series of nipype, major series of nibabel (#2021)
Changes proposed in this pull request
This PR updates the external dependencies listed in installation.rst for standalone installation to match the packages and versions found in the Dockerfile.
There is also a typo fixed in CONTRIBUTING.md
This should close issue #2008, which I raised today.
The need for an updated version of ICA-AROMA is pretty clear, as fmriprep now uses a flag that doesn't exist in the older version. I used the version and download link used in the Dockerfile - if there's a more appropriate permalink that might be better.
The bids-validator version has been updated to match the Dockerfile.
I can't see any indication that c3d is installed in the Dockerfile, so I removed it as a dependency.
Documentation that should be reviewed
This is a documentation only fix, which modifies docs/installation.rst and CONTRIBUTING.md