-
Notifications
You must be signed in to change notification settings - Fork 60
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
Next connectivity scripts #1063
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1063 +/- ##
==========================================
+ Coverage 69.46% 69.52% +0.06%
==========================================
Files 447 448 +1
Lines 24052 24070 +18
Branches 3291 3290 -1
==========================================
+ Hits 16708 16735 +27
+ Misses 5946 5942 -4
+ Partials 1398 1393 -5
|
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.
Everything looks alright, but I asked @gagnonanthony to review the PCA script since he wrote it (and maybe even re-test it himself). I would wait for his OK
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.
Thanks for reformatting the scil_connectivity_compute_pca
script! Clearly there was some work to be done. I've tested it quickly and compared it to the previous version, and it seems that the typical edges selection is not working correctly. Using the all_edges
or not returns the same results.
df[df == 0] = 'nan' | ||
|
||
# Standardized the data. | ||
common_edges_mask = np.sum(matrices_per_metric[0]) != 0 |
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'm unsure about this; the common edges are supposed to be across all input subjects. Using the --all_edges
flag returns the same results as if we don't use it.
Hello @EmmaRenauld, Thank you for updating ! There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-12-17 18:40:54 UTC |
Sorry! Should be fixed |
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 confirm the results are now identical to before! Thanks @EmmaRenauld
|
||
plt.figure() | ||
plt.imshow(common_edges_mask) | ||
plt.title(common_edges_mask) |
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.
This is returning an error (I guess it's a typo!). I like the matrix visualization of the common connections, by the way, great addition!
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!
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.
GTG
Cleaning:
scil_connectivity_compare_populations: already clean! simply trying to clarify the help!
scil_connectivity_normalize: already clean! simply trying to clarify the help!
scil_connectivity_reorder_rois: already nearly clean! simply trying to clarify the help! Some code was written twice; simplified.
scil_connectivity_compute_pca: This script was not yet written with scilpy's standard. Bigger cleaning.