-
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
[ENH] Add mrds scripts part 3 #1046
Conversation
Hello @arnaudbore, Thank you for updating !
Comment last updated at 2024-11-12 16:11:54 UTC |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
- Coverage 68.89% 68.77% -0.13%
==========================================
Files 434 436 +2
Lines 22646 22754 +108
Branches 3078 3094 +16
==========================================
+ Hits 15603 15649 +46
- Misses 5727 5793 +66
+ Partials 1316 1312 -4
|
|
||
all_metric = mrds_sum[0] | ||
for curr_metric in mrds_sum[1:]: | ||
all_metric += curr_metric |
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.
Hmm. Isn't it just all_metric= np.sum(curr_metric, axis=0)
?
But it's only used for non-zero. Is there a possibility that the metrics will cancel each-other?
Something like that could be safer?:
non_zeros = np.sum(np.non_zeros(curr_metric), axis=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 just add a absolute value so it does not cancel each-other, also this method mimics afd_along_streamlines.py.
from scilpy.tractanalysis.grid_intersections import grid_intersections | ||
|
||
|
||
def mrds_metrics_along_streamlines(sft, mrds_pdds, |
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 think that this method is not mrds-related. It's just dividing data.
It could be, in the main script:
mrds_sum, weights = \
mrds_metric_sums_along_streamlines(sft, mrds_pdds,
metrics, max_theta,
length_weighting)
weighte_mrds = weight_values(mrds_sum, weights)
Where weight values is function that divides by the weight where non-zero, somewhere in volume tools or something.
StatefulTractogram containing the streamlines needed. | ||
mrds_pdds : ndarray (X, Y, Z, 3*N_TENSORS) | ||
MRDS principal diffusion directions (PDDs) of the tensors | ||
metrics : list of ndarray (X, Y, Z, N_TENSORS) |
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 think metrics
should be metric
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 aggree it's "a" list but reading it I feel like metrics make sense here.
weight_map = np.zeros(metrics[0].shape[:-1]) | ||
min_cos_theta = np.cos(np.radians(max_theta)) | ||
|
||
all_crossed_indices = grid_intersections(sft.streamlines) |
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 am pretty sure that our "uncompress" method should be used somewhere here. @frheault probably knows this better than me.
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.
Maybe, this version is copy paste (kinda) of afd_along_streamlinespy
Quick description
Script to get MRDS metrics along a bundle - fixel based metric.
...
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist