review #1

[ltetrel]

Thank you @mathieuboudreau for agreeing to review this submission. You can find our guidelines for review here. When your review is completed, please leave a comment on this issue saying so. If there are unresolved issues that you believe to be impeding the acceptance of the repository, please list them here.

[mathieuboudreau]

I'll be covering notebooks #0-3 at first.

[mathieuboudreau]

Review notes will be updated below, I'll comment again when I think it's done.

--Finished first review--

General recommendations

• Consider either linking the binder button directly to the first notebook or to the notebooks folder.
• Would benefit from some general cleanup in the repo (e.g. multiple "requirements.txt" files (in tutorials/ and in in binder/), makes it difficult to know which one is used in an environment).
• Clarifying somewhere the Python version could be beneficial to the students (i.e. let them know it's 3.7.1 or >=3.7.1 so that users who have only ever used 2.7 will know they need to look up updated syntaxes) . This could be shown in either the README or in one of the first notebooks.
• I believe the proper name for the repo2data file herehttps://github.com/neurolibre/brainiak-tutorials/blob/master/binder/_data_requirement.json shouldn't begin with an underscore, correct @ltetrel ?
• In the requirements.txt and apt.txt file, a better practice would be to specify a fixed version for all the packages (using package==x.x.x for requirements.txt, and package=x.x.x for apt.txt). An alternative would be to specify a version >= so that you can always have the latest version, but at least you'll know what versions all the packages were originally were you need revert back to them.

Notebook # 0

• For easier long term maintenance of this notebook, I'd recommend you divide the imports by section in terms of notebook number (or even, one cell per all import for each notebook). That way if you update a notebook's import in the future, you'll know where to add them in this test file, and if one of them fails, you'll have better information of which notebook(s) will be affected.
• A warning is thrown for sklearn; one of the imported module will be depricated in version 0.23 (currently on 0.21). If you don't fix the version of sklearn installed, some of your users may have problems in the issue if a docker image cache is cleared and/or new build is executed. Some text from yourselves acknowledging this warning to reassure the user that this warning is expected and won't impact their tutorial experience may be valuable.

Notebook # 1

• I would move the "Contributions" link to the Table of Contents section.
• "Congratulations, if you are viewing this Jupyter notebook, you have already acquired many of the skills necessary to excel in this course and you are well on your way to learning cutting-edge methods for cognitive neuroscience!" Because the NeuroLibre/Binder process is just a click of a button instead of an installation procedure (which I assumed it might have been before?), this introductory sentence may benefit from a slight revision.
• "Goal of this script": Since you already called this a Jupyter notebook, "script" should be replaced by "notebook"
• "Here're" should be replaced by "Here are"
• "python" should be capitalised when describing the language.
• "by the end of this course you will be an expert in not only these useful skills" I would recommend saying that "...you will be proficient in not only these useful skills..."
• Before the packages are imported, a useful header like "Import necessary packages" would make sense, as it's not really part of the "Resources" section, which is used to link to external resources.
• "you will see that sim.mask_brain take": "take" should be "takes"
• "Note, you can also do this by typing [SHIFT] + [TAB] while the cursor is hovering over a function name." This does not appear to work for me as described. I need to select the entire function name (function name + package name) for this to work, and it only gives me the expected arguments and not the whole help information.

Notebook # 2

• ", is now doable" remove that comma.
• "To use all three aspects" modify to "To use all three of these"
• "Using this script you will learn" change to "This script will teach you how"
• "you will learn the following" change to "you will learn how to do the following"
• "This dataset will heretofore be referred to as the "VDC" dataset." - everywhere else in the document you refer to the dataset in lowercases "vdc", so maybe change it here?
• Section 1: "nibabel: Read fMRI data into python arrays." - capitalize Python. **(happens throughout this notebook)
• Section 1: "Machine Learning methods." - Learning should start with a lower case
• Section 1.1: "across NIfTI and matlab file" - "matlab" should be all capitalized ("MATLAB") **(happens throughout this notebook)
• Section 1.1: "Note on data file paths:" This note appears to be outdated with in NeuroLibre, which will always have the same data folder.
• Exercise 1: Missing an "A:" cell for the answer
• Section 2: "Self-study: Navigate through the folder that contains these data (defined in vdc_data_dir) to get an understanding of the file structure. Open up the files and look at their contents. Be an explorer!" using the current setup, that directory is unreachable in the BinderHub session. Maybe @ltetrel could give you a hand in how to modify your data_requirements.txt file so that the data is stored in the current root visible folder of the github project when the Jupyter session is loaded.
• Section 2.1: The acronym "TR" was never defined as repetition time.
• Section 2.1: "Recommendation: Because Python suppresses output (when there is no error), you may want to include print statements at the end of cells to indicate to you when a cell has executed all lines of code" Actually, in Jupyter, the left of the cells show a star symbol (*) while processing, and a number (e.g. 6) when it is done, so this information could be shared too.
• "Exercise 3: Plot the stimulus presentation for runs 2 and 3 for this subject." The run ID selected by default was already 2, so either that one need to be changed to 1, or this exercise should say "for runs 1 and 2".
• "Excercise 4" is either missing, or the number was skipped accidentally and numbers 5-9 should be reduced by 1.
• Section 3.1: "# Plot a voxel value (in this example of voxel 100) through time" The example actually gets voxel 50.
• Section 4: "In machine learning normalization is a" please put a comma after "learning"
• Section 4: "In fMRI we often" please put a comma after "fMRI"
• Section 4: Running the cell in the default state outputs a messy string message because of how you've formatted your comments and commented out code with doctrings. A better & cleaner option would be to use the hash sign at the beginning of each line instead.

Notebook # 3

• "in the recent past" is not a proper expression. Maybe change the sentence to "Up to just a few years ago, email servers did not have spam filters".
• "This notebook, will walk you through" remove that comma
• "of extracting fMRI signal," from what? raw data?
• "Goal of this Script": Some of the bullet points have periods at the end, while some do not, so change either of them to be consistent.
• "matlab" should be written capitalized (e.g. "MATLAB") throughout.
• "Usage of hstack and vstack:" the two markdown links in this section are not displaying properly (we're seeing the markdown syntax and not a clickable link"
• "Recommendation: Creating and using functions is an efficient way to program." Maybe add a mention that although functions will be written in the Jupyter notebook for this tutorial series for ease of use, a more recommended approach is to contain function in individual external files.
• Section 2.3.3: "we have 3 equally represnted sets of" typo in the word "represented"
• Section 2.4: "If you do have issues, change how many cores/memory you are using when you launch this notebook." This isn't an option in the NeuroLibre binder setup, is it @ltetrel?
  Exercise 2: Tell the reader that the function should also output models and scores
• Section 3: "The discovery of the Fusiform Face Area (FFA; Kanwisher, McDermott, and Chun, 1997; see also McCarthy, Puce, Gore, & Allison, 1997) was a landmark discovery." the sentence is repetitive. Try "he discovery of the Fusiform Face Area (FFA; Kanwisher, McDermott, and Chun, 1997; see also McCarthy, Puce, Gore, & Allison, 1997) was groundbreaking for our field." or something like that.
• Section 3: "In this notebook, you" should be replaced with "In this section, you", since we already did other things in this notebook.
• Exercises 4 and 5: when trying the exercises, I always got the same scores as the examples above ([0.88, 0.8933333333333333, 0.8466666666666667]) , not sure if whatever mistake I made might be a common one that users experience, just a FYI.

[mathieuboudreau]

This review has been completed and individual issues for each notebook have been opened.

[manojneuro]

@ltetrel this issue is now resolved with PR #16

[ltetrel]

@mathieuboudreau can you check the PR since your are responsible for this review ?

[ltetrel]

@mathieuboudreau regarding your comment about the data_requirement file, you are right. But since I already downloaded the files manually on the server, there is no need for such a file, so I hid it.
I will check later if the requirement is working (when the certificates will be updated), just to be consistent with our production workflow.