-
Notifications
You must be signed in to change notification settings - Fork 6
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
[ENH] start using mypy and updating types #86
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
- Coverage 92.40% 91.84% -0.56%
==========================================
Files 11 11
Lines 487 503 +16
==========================================
+ Hits 450 462 +12
- Misses 37 41 +4
|
tools/download_templates.py
Outdated
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.
not sure it makes sense to check for type annotation in tooling scripts...
@htwangtw Let me know if you have questions. |
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.
I certainly learned a lot reading this PR and thank you for doing this!!!
group_mask: Union[str, Path, Nifti1Image], | ||
atlas_image: Union[str, Path, Nifti1Image], | ||
) -> Tuple[np.ndarray, np.ndarray]: | ||
correlation_matrix: np.ndarray[Any, Any], |
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.
Is this how to declare the dimension of the numpy array? That's cool
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.
I need to investigate better how to use types with numpy, this is a bit of a brute force approach.
To be honest I had to learn a lot to make it happen!!! |
merge conflict resolved let's see what CI says |
merge conflicts resolved |
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.
LGTM and this is really cool. Merging....
TODO:
Any
(probably in a follow up PR)