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] Include custom ODFs #33

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

kellychang4
Copy link

@kellychang4 kellychang4 commented Dec 4, 2024

Add argument for sh basis types. Force legacy basis types to FALSE.

Starting edit to incorporate custom ODF files into workflow. Need to separate ODF model estimation from tracking procedure.

@kellychang4 kellychang4 changed the title include argument for sh basis types [ENH] Include custom ODFs Dec 4, 2024
@36000
Copy link
Collaborator

36000 commented Dec 4, 2024

I wonder if odf_model should be renamed fodf_model and made its own task in tractography.py in the tasks folder. then it could be from an image file or a string, and that method in the tractography task folder either calls the appropriate data task method if its a string or calls the imagefile function if its an imagefile, similar to 'brain_mask'. this could also just be handled with if statements in the tractography method. I will work on this tomorrow

@kellychang4
Copy link
Author

[...] made its own task in tractography.py in the tasks folder. then it could be from an image file or a string, [...]

I think if we go down this route to allow for custom fODF images that it would unlink these two procedures. I am in support of making them two separate tasks.

@arokem
Copy link
Contributor

arokem commented Dec 10, 2024

After a conversation with @36000, I think that maybe we remove the "legacy" and "basis_type" options here, and leave that up to the user. Then, this code would still allow users to provide custom ODFs, but they would have to make sure that the coefficients are for the "descoteaux" basis set, and they'd have to do their own conversion in advance for this. @kellychang4 : WDYT?

@kellychang4
Copy link
Author

That is fine with me, so long as it is written somewhere!

@arokem
Copy link
Contributor

arokem commented Dec 10, 2024

For future reference (and maybe to be added to DIPY at some point?), we've discovered this function: https://github.com/scilus/scilpy/blob/master/scilpy/reconst/sh.py#L560, which can be helpful for users of this functionality.

@36000 36000 added this to the pyAFQ Hackathon 12/19/2024 milestone Dec 19, 2024
@36000 36000 marked this pull request as ready for review January 8, 2025 19:36
@36000
Copy link
Collaborator

36000 commented Jan 8, 2025

@arokem once these tests pass, this is ready to review/merge. kelly has signed off too

AFQ/definitions/image.py Outdated Show resolved Hide resolved
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