search

bids-apps / rs_signal_extract

BIDS App for resting state signal extraction using nilearn.
Apache License 2.0
6 stars 6 forks source link

Move atlas download to Dockerfile #3

Closed chrisgorgo closed 8 years ago

chrisgorgo commented 8 years ago

The atlas downloaded here: https://github.com/BIDS-Apps/rs_signal_extract/blob/master/main.py#L34 should be part of Dockerfile not the analysis script. It's important from the reproducibility point of view. Currently running the same rs_signal_extract image on multiple occasions can yield different results if the atlases change (unless the downloader checks md5sums).

Additionally the App should be able to run without internet access and the currently the atlases are saved in /root/ which is not writable.

GaelVaroquaux commented 8 years ago

I disagree, dear sir: for now, in the dumbed bown version of an analysis that this App is providing, such an option is feasible. But later we will want to add an option as to which atlas is applied. In this situation, embarking the atlas in the dockerfile will no longer be an otion.

chrisgorgo commented 8 years ago

I'm happy to have multiple atlases bundled in the Dockerfile and letting the user select one or giving the user an ability to provide their own atlas. I just want the analysis performed by the app to be only dependant on the input data - not on the outputs of an HTTP call that could be something today and something else another day.

It should be easy to implement - you can just download all of the atlases you want to provide to users in the Dockerfile. nilearn will pick them up using your superb caching system when the app will actually run.

chrisgorgo commented 8 years ago

Another purely pragmatic reason to keep it inside the container image is caching. We would like to avoid having to download the atlases each time this analysis runs.