[ENH] Added RUMBA-SD to data.py #1129
Conversation
|
Hello @chiuhoward! 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 2024-04-23 20:11:56 UTC |
|
Maybe we could break this up into several tasks. One would be |
36000
changed the title
|
@arokem this is ready for review/merge |
AFQ/tasks/data.py
Outdated
| @as_file(suffix='_odfmodel-RUMBA_desc-diffmodel_dwi.nii.gz') | ||
| @as_img | ||
| def rumba_params(rumba_model, data, brain_mask): | ||
| """RUMBA SHM Parameters""" |
There was a problem hiding this comment.
| """RUMBA SHM Parameters""" | |
| """Fits the RUMBA SD model. """ |
Would be good to finish this docstring, including a "Parameters" and a "Returns" sections.
There was a problem hiding this comment.
@36000 would this docstring work? I don’t think there’s much room for user configuration in this function, and I’m not too sure what meta does… took a stab at defining what the function returns by looking at QBallTP.py
"""
Takes the fitted RUMBA-SD model as input and returns the spherical harmonics coefficients (SHM).
Parameters
----------
rumba_fit: use output from rumba_fit as input for rumba_params
Returns
----------
rumba_shm: spherical harmonics coefficients from an ODF without the isotropic and anisotropic diffusion components
"""
There was a problem hiding this comment.
This is a part of the task system so we typically only have parameters when theres user input via named arguments, as you point out Howard. We also typically don't have returns because we usually just describe it in the main docstring. These task functions are all only called by pimms. So I think we could do something like this is good:
Takes the fitted RUMBA-SD model as input and returns the spherical harmonics coefficients (SHM).
AFQ/tasks/data.py
Outdated
|
|
||
|
|
||
| @pimms.calc("rumba_f_csf") | ||
| @as_file(suffix='_odfmodel-RUMBA_desc-CSF_dwi.nii.gz') |
There was a problem hiding this comment.
Maybe use probseg as suffix? https://bids-specification.readthedocs.io/en/stable/glossary.html#probseg-suffixes
There was a problem hiding this comment.
@arokem if i’m understanding the docs correctly, each filename can only have one suffix after all of the keyword-value pairs, so the correct name would then be
@as_file(suffix='_odfmodel-RUMBA_desc-CSF_probseg.nii.gz’)?
There was a problem hiding this comment.
Thats correct @chiuhoward !
There was a problem hiding this comment.
@36000 : How do you more generally feel about using probseg as a suffix here? Is it inconsistent with our usage elsewhere?
There was a problem hiding this comment.
I think its correct! I just use dwi when I don't have anything else to put.
There was a problem hiding this comment.
Sorry one last question @36000, probseg should be the suffix not just for @pimms.calc("rumba_f_gm”) but also for @pimms.calc("rumba_f_csf”), @pimms.calc("rumba_f_wm”) and @pimms.calc("rumba_params”)?
There was a problem hiding this comment.
just for rumba_f_gm, rumba_f_csf, rumba_f_wm
|
LGTM, Ariel? |
AFQ/tasks/data.py
Outdated
| @@ -4,6 4,8 @@ | |||
|
|
|||
| from dipy.io.gradients import read_bvals_bvecs | |||
| import dipy.core.gradients as dpg | |||
| from dipy.data import default_sphere | |||
| from dipy.data import default_sphere | |||
There was a problem hiding this comment.
Duplicate import
| from dipy.data import default_sphere |
|
Actually, now that I look at this, this also removed the refactoring I did so that we could use the SHM from disk from previous runs. Let me see If I can do this merge |
|
I think this should be good to go! @chiuhoward look it over and make sure i didnt delete any changes u made recently. |
|
@36000 looks good! suffixes and docstrings were all that I changed and they’re there |
|
OK, once the docs finish I will merge, unless @arokem has any more comments. overall, good work everyone! |
|
Yeah - looks great! When you get a chance, maybe you can write a PR that has an example? @36000 showed me some really impressive tissue type maps from this model using the Stanford HARDI dataset. Might be nice to show slices from these as part of that. |
Tried to add RUMBA-SD based on https://docs.dipy.org/stable/examples_built/fiber_tracking/tracking_rumba.html and https://docs.dipy.org/stable/reference/dipy.reconst.html#dipy.reconst.rumba.RumbaSDModel