Skip to content
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 Callosum ROIs support #538

Merged
merged 18 commits into from
Dec 14, 2020
Merged

Conversation

bloomdt-uw
Copy link
Collaborator

@bloomdt-uw bloomdt-uw commented Oct 9, 2020

#494

@pep8speaks
Copy link

pep8speaks commented Oct 9, 2020

Hello @bloomdt-uw! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-12-14 21:48:15 UTC

AFQ/api.py Outdated Show resolved Hide resolved
@36000
Copy link
Collaborator

36000 commented Oct 12, 2020

@bloomdt-uw #539 should fix the anatomical plot position problems by saving the plots individually.

@@ -2006,7 +2006,15 @@ def bundles_to_aal(bundles, atlas=None):
"UNC_L": [['leftanttemporal'], ['leftuncinatefront']],
"UNC_R": [['rightanttemporal'], ['rightuncinatefront']],
"ARC_L": [['leftfrontal'], ['leftarctemp']],
"ARC_R": [['rightfrontal'], ['rightarctemp']]}
"ARC_R": [['rightfrontal'], ['rightarctemp']],
"AntFrontal": [None, None],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jyeatman : what are the correct endpoints for each one of these segments of the CC?

@36000
Copy link
Collaborator

36000 commented Dec 7, 2020

I added issue #614 to remind us to figure out where the callosum endpoints are, but I don't think that is high priority.

@36000 36000 marked this pull request as ready for review December 8, 2020 21:31
@36000 36000 changed the title WIP: Enh Add Callosum ROIs support Enh Add Callosum ROIs support Dec 8, 2020
@36000
Copy link
Collaborator

36000 commented Dec 8, 2020

@arokem this is ready for review.

@bloomdt-uw for your checklist, I moved the endpoint atlas to an issue, so we can merge this now and find out what the correct endpoints are later. I am not sure what the last part of your checklist is referring to, however this may merit its own issue.

I got rid of the test and instead have a callosum example. I also modified some of the examples to use less streamlines in tractography to save space, which I think was causing problems. I also modified the 'organize stanford data' function to optionally delete any afq derivatives folder in the stanfords bids directory. This is because the examples use the same BIDS directory and were using each others derivatives.

@bloomdt-uw
Copy link
Collaborator Author

The task to explore other datasets was meant to be a quality control check

@arokem
Copy link
Collaborator

arokem commented Dec 8, 2020

Looks great. My main comment here is that we probably want to prevent users from asking for these new callosal bundles and also for FP/FA in the same AFQ object. The reason is that these are co-located and our current code makes sure that each streamline is classified into only one bundle. This would mean that (for example) the occipital callosum and FP would be competing for the same streamlines, which doesn't make sense.

@36000
Copy link
Collaborator

36000 commented Dec 10, 2020

@arokem I integrated your advice. This is what the example produces now.
image

Copy link
Collaborator

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! One more small suggestion to follow up on the recent change.

AFQ/api.py Show resolved Hide resolved
@36000
Copy link
Collaborator

36000 commented Dec 11, 2020

I added #616 for exploring different datasets.

36000 and others added 2 commits December 14, 2020 13:47
Co-authored-by: Ariel Rokem <arokem@gmail.com>
Co-authored-by: Ariel Rokem <arokem@gmail.com>
@arokem arokem merged commit ddf7619 into yeatmanlab:master Dec 14, 2020
@bloomdt-uw bloomdt-uw deleted the enh-callosum-rois branch December 29, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants