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

Slf123new #997

Merged
merged 26 commits into from
Aug 31, 2023
Merged

Slf123new #997

merged 26 commits into from
Aug 31, 2023

Conversation

jyeatman
Copy link
Member

added SLF 1/2/3
@arokem will you update this with files on figshare?

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! A couple of thoughts: I think that it is helpful to show users how to download some ROIs to a well-defined location in their file system and operate with these files. I think this example achieves that well. That said, I think that we will also want something a bit more automatic to do when we are running code to find these different SLF sub-segments in real use-cases, because I don't think we'll want to copy and paste all those paths into our scripts. So, I do think that we'll want to implement some convenience functions for working with these ROIs, in addition to this example. Something along the lines of this suggestion on the cerebellar peduncles PR. WDYT?

examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
@jyeatman
Copy link
Member Author

@arokem I totally agree. Can you help with this? To summarize:

  • we should implement some helper functions that go in the main code base to make this segmentation a default like VOF is.
  • in this tutorial we can include commented out versions of those helper functions but keep the full tutorial for teaching sake but also referring to the "correct way"

@36000 the visualization code at the end crashed with the following error. Also, the montage just had renderings of an axial view in individual subjects with no montage combining them. Also, I want these for all the pathways not just one. Can you help?

Traceback (most recent call last):
File "/Users/jyeatman/git/pyAFQ/examples/tutorial_examples/Add_Custom_Bundle.py", line 189, in
#############################################################################
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'L_SLF1'

@jyeatman
Copy link
Member Author

@arokem @36000 I think this is ready to merge. I would like the example to show the montage figures and also have the interactive HTML for 2 subjects. Would that work?

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.

This is great! Some suggestions and comments below.

AFQ/api/group.py Outdated Show resolved Hide resolved
examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
])


roi_urls = ['https://github.com/yeatmanlab/AFQ/raw/c762ca4c393f2105d4f444c44d9e4b4702f0a646/SLF123/ROIs/MFgL.nii.gz',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should explain what's going on here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Emphasizing perhaps that these ROIs can be files that the user created themselves, but that in this case they are simply files stored somewhere on the internet (which happens to be another software library with a very similar name - this could get confusing for someone who doesn't know all the history...)

Copy link
Member Author

Choose a reason for hiding this comment

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

@arokem I just added this. Please edit as you see fit and I'll merge

examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
examples/tutorial_examples/add_custom_bundle.py Outdated Show resolved Hide resolved
@36000 36000 mentioned this pull request Aug 30, 2023
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.

I've done another round of editing some of the text and formatting in the examples. I believe this is ready to merge, so I will go ahead with it. If we need to do more fixes, we can follow up on a separate pr.

@arokem arokem merged commit 763e0fc into yeatmanlab:master Aug 31, 2023
0 of 8 checks passed
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.

3 participants