Code review - README & preprocessing

[cmaumet]

Hi @AlexBowring! As agreed I have had a look at your code. So far I have only looked at the preprocessing as that is what is currently described in the README. I have a few suggestions about the config files.

I'll let you update the README with the info on how to run the analyses and then I'll have another look.

Code review By Camille. Date: Sept 28, 2020. Latest commit: 4612917

General notes

README

• In “Config files”, when first referring to config.py and config.mat would be nice to provide a relative path and/or a direct link to those files within the repository.
• I think the paragraph describing how to fill in config.py can be removed and instead the description of each variable added as a comment in the actual config file.
• For clarity, in “Preprocessing the data with fMRIprep” we can remove mentions of “PATH_TO_SC2_DIR” and instead use a relative path: e.g. /PATH_TO_SC2_DIR/src/process_fmriprep.py becomes ./src/process_fmriprep.py
• Note: So far README is only including info on how to run the preprocessing.
• Ideally, to avoid incidentally leaking personal information, we would leave all the config values empty (for the user to setup) and remove the local copy from git versionning.

Minor comments

• Setting up config.mat would be easier if it was a text file (.m) rather than a binary file (.mat). In that case we could use comments in the .m file to explain what each variable is (as for config.py above).
• Comment "(This is the template README.md for this template project sharing repository; please see HOWTO.md for usage guidelines for this repo.)" should be deleted.

Links when reading through the code

Preprocessing. Entry point: process_fmriprep.py

• run_fmriprep.py

[AlexBowring]

Hi @cmaumet ,

Thank you for the comments so far! I have just updated the README with descriptions of how the analyses were carried out, see the latest commit: d1920bd

I'll work through your feedback about the README soon, but hopefully this update means you are not held up in continuing the review. Let me know if there are any problems or bits that don't make sense!

[AlexBowring]

Hi @cmaumet,

In the latest commit 89b7be4 I have addressed some of your suggestions about the README and config files, including:

• In config files, I now provide relative paths to config.py and config.mat.

• I have now removed the description of how to fill in config.py, and these instructions are provided in the actual config.py file. Note that I have pointed users to the file in the README, and still left the link to the FreeSurfer license file.

• I have removed all of the instances of "PATH_TO_SC2_DIR" in the file paths, and changed them to a relative path. e.g. /PATH_TO_SC2_DIR/src/process_fmriprep.py is now ./src/process_fmriprep.py

• I have removed the "(This is the template README.md for this template project sharing repository; please see HOWTO.md for usage guidelines for this repo.)" comment from the top of the README

• As already mentioned in my previous comment, I have added descriptions of how each of the analyses were carried out to the README

While I agree that for Matlab/Octave, a config .m file would be clearer than a binary .mat, I am not 100% sure on whether workspace variables can be loaded from a .m file in the same way they can be from a .mat file as described here.