Merge from Haji's fork into main fork #125
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e new cache files
noahbenson
commented
Dec 10, 2024
| def load_allreports(region, | ||
| reports_path=None, | ||
| include_mean=True, | ||
| sids=subject_list): | ||
| sids=None): |
There was a problem hiding this comment.
sids=None is a good way to set up the argument, but there needs to be an if statement inside the function:
if sids is None:
sids = subject_list| """Loads all reports for a region and returns a dataframe of them. | ||
|
|
||
| This runs `load_report` over all raters, subjects, and hemispheres and | ||
| returns a dataframe of all the reports. If a report file is not found, | ||
| then the row is left with NaNs indicating missing data. | ||
| """ | ||
| from .config import raters_by_region |
| @@ -1,5 +1,5 @@ | |||
| ''' | |||
| The hcpannot package contains code for annotating the HCP 7 Tesla Retinotopy | |||
| The hcpannot package contains visualization for annotating the HCP 7 Tesla Retinotopy | |||
There was a problem hiding this comment.
Not sure how this got here, but not needed.
Comment on lines
+1
to
+3
| from . import plot_annot_properties as visannot | ||
| from . import plot_contours as viscontours | ||
| from . import utils as utils |
There was a problem hiding this comment.
This is a fine way to organize your __init__.py file, but I would just name the modules visannot.py, viscontours.py, and utils.py. You can still import them, but people will be surprised if they have two names.
There was a problem hiding this comment.
Big picture things to do:
- Add docstrings to functions. These don't need to be fancy especially if the function is made to be called internally and not by a user; one line is usually fine for those. For functions meant to be called by the user, look at Numpy's guide.
- Pretty minor: Best to word-wrap lines at 79 or 80 columns.
- Divide the file into two sections (this is sort of already done): at the top is the private functions not meant to be called externally, and at the bottom are the public functions with longer docstrings. Section these off with some comments.
There was a problem hiding this comment.
Review this code before our next meeting—we want to have a better idea of how it gets run, and we can maybe walk through an example of how it is intended to work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.