-
Notifications
You must be signed in to change notification settings - Fork 429
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
[WIP] NF: Population specific bundle atlas creation #2425
base: master
Are you sure you want to change the base?
Conversation
Hello @drombas, Thank you for updating ! Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated at 2021-08-17 10:47:06 UTC |
Codecov Report
@@ Coverage Diff @@
## master #2425 +/- ##
==========================================
+ Coverage 84.49% 84.73% +0.23%
==========================================
Files 125 127 +2
Lines 16624 16909 +285
Branches 2702 2769 +67
==========================================
+ Hits 14046 14327 +281
- Misses 1911 1913 +2
- Partials 667 669 +2
|
Thank you @drombas for this PR. Let us know when it is ready for a first review. Thank you |
Hi @skoudoro ! Tests finally look mostly ok so yes, please, have a look at it! |
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 looks really interesting. I have a couple of small comments.
@@ -420,3 +425,71 @@ def read_img_arr_or_path(data, affine=None): | |||
affine = data.affine | |||
data = data.get_fdata() | |||
return data, affine | |||
|
|||
|
|||
def show_bundles(bundles, fname, colors=None, opacity=None, linewidth=None): |
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 wonder whether this shouldn't go into dipy.viz
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 putted there cause I use it to save the output as png files but makes sense to move it to dipy.viz
. @skoudoro what do you think?
dipy/atlasing/bundles.py
Outdated
raise ValueError("Subjects cannot be duplicated") | ||
|
||
print(str(len(subjects)) + " subjects to be processed:") | ||
print(subjects) |
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 can be pretty long, no?
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.
Yes but I found it useful as a safety check. Using logger.debug() might be a good option?
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.
@@ -0,0 +1,506 @@ | |||
"""Atlasing module: utilities to compute population specific bundle atlases. |
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.
population-specific streamline-based bundle atlases.
"""Atlasing module: utilities to compute population specific bundle atlases. | ||
|
||
Available functions: | ||
get_pairwise_tree: computes the matching pairs for a given number of items. |
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.
items or bundles?
combine_bundle: combines two bundles into a single one using different | ||
streamline combination methods. | ||
|
||
compute_bundle_atlas: given a list of input bundles computes the population |
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.
Does this create a whole-brain atlas from given bundles? eg: merging all bundles to create one whole brain atlas. Or does it create one atlas bundle per bundle type? I think it's the second one. Maybe just rephrase it.
"""Pairwise tree structure calculation. | ||
|
||
Constructs a pairwise tree by randomly matching the indexes of a given | ||
number of items. The computed index structure is intended to be used for |
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.
Let's just use the word 'bundle' since it is a bundle-specific atlas or do you think this is more generic and could be used for other purposes? If it's the second, then maybe we need to move it to utils?
# If n_item is odd duplicate one of the others | ||
if np.mod(n_item, 2) == 1: | ||
# avoid removing again an item twice (index == 0) | ||
lonely = np.random.randint(1, n_item) |
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.
How about the variable name single
?
from dipy.atlasing.bundles import compute_atlas_bundle | ||
|
||
|
||
class DiscreteBundleAtlasFlow(Workflow): |
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 we should drop the word 'Discrete'.
|
||
Given several segmented bundles as input, compute the atlas by | ||
combining the bundles pairwise. | ||
|
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.
Add some more details
distance : str, optional | ||
Distance metric to be used to combine bundles. Default is 'mdf'. | ||
The 'mdf_se' distance uses only start/end points of streamlines. | ||
comb_method : str, optional |
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.
Same comment as in the other functions.
space=Space.RASMM) | ||
save_trk(new_tractogram, file, bbox_valid_check=False) | ||
return atlas, atlas_merged | ||
return atlas |
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.
You can just return atlas, atlas_merged
atlas_merged
can be None if there isn't one. This way the function output will be consistent (returns fixed number of objects).
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.
Will solve the issue in the workflow as well where you have written the same function call twice with if and else conditions.
bundle_names=bundle_names, | ||
model_bundle_dir=model_bundle_dir, | ||
out_dir=out_dir, | ||
merge_out=merge_out, |
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.
You can call it one time and just always have it return two outputs. See my comment above in compute_atlas_bundle
function.
rm_small_clusters=1, | ||
qbx_thr=[5]) | ||
|
||
points, offsets = unlist_streamlines(static) |
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.
Why unlist and relist here?
When I run it and generate average tract using TRACULA training data (33 subjects), I found interesting error only happens in bilateral cab_PP (cingulum-angular bundle). I guess it's due to the irregularity of each subject's tract shape, but cannot clearly figure out what happened.
|
Hi @iPsych, |
1419292
to
ca6268a
Compare
5935e1e
to
765963e
Compare
Hi! This is part of GSoC 2021.
Goal
Given the segmented bundles of several subjects as input, compute a bundle atlas that is representative of that population.
Implementation
The input bundles are combined in pairs following a tree structure until a single bundle is obtained:
The main logic is implemented in
atlasing.bundles.compute_bundle_atlas()
and consists of these steps:atlasing.bundles.get_pairwise_tree()
atlasing.bundles.combine_bundles()
, which implements several methods to match the streamlines and combine them into a single bundle.Comments
This is WIP as we are still testing different approaches and need to reach a final decision on:
List of tasks: