-
Notifications
You must be signed in to change notification settings - Fork 34
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
[DOC] add interactive bundle viz to OR example #861
Conversation
A couple of thoughts here:
Also, noticed that the ORs in this subject also have the additional more anterior section: |
I think putting the montage only is a good idea. The cropped endpoint ROIs would fix the anterior section problem |
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.
A couple of comments here.
I didn't realize that this also involves a new set of OR ROIs! We should ask @SendyCaffarra and @jyeatman for some feedback on this new definition of the anterior portion of the OR, which should help get rid of the anterior branch that we have been seeing in some subjects.
@@ -593,9 +593,9 @@ def read_templates(as_img=True, resample_to=False): | |||
] | |||
|
|||
or_remote_fnames = [ | |||
"26831630", | |||
"36514170", |
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.
Just making sure I understand this: These are the new edited ROIs for thalamus?
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 and this works well in test datasets
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.
Do we have a way to refer to different versions of this data on figshare?
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 not sure what you mean by this. Note that the new OR ROIs are on my figshare but the old ones are not.
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.
Oh! Gotcha. I think that means we are good to go here.
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 mean up to the sphinx stuff, which I can sort out. Let me pull this rebased thing down and fiddle with that.
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.
Oh - I see you are sorting that out too in a8a12a1
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 still need to test that
examples/plot_optic_radiations.py
Outdated
my_afq.combine_bundle("L_OR") | ||
display(Image(filename=montages[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.
This call to display
does not embed this in the html that sphinx generates. I think that we need the sphinx rst to encode the figure. Something like this code: https://github.com/yeatmanlab/pyAFQ/blob/master/examples/plot_afq_api.py#L181-L184 should do it.
0041efc
to
797cc26
Compare
@arokem I am not sure if the sphinx integration is good to go, but otherwise this is good. for example the new thalamus ROI is tested on hbn, hcp, and hardi, and it gets rid of the anterior endpoint problem. |
@36000 : when you get a chance, could you please rebase this on top of current master? |
797cc26
to
a8a12a1
Compare
Looks like it works now! |
No description provided.