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

Add option to compute dwi sh #41

Merged
merged 8 commits into from
Sep 7, 2021
Merged

Conversation

ppoulin91
Copy link
Contributor

@ppoulin91 ppoulin91 commented Mar 12, 2021

Optional --compute_sh option, useful to compute metrics from the signal ODF in the future.
Question:

  • Should we have separate parameters for --basis and --sh_order, or do we keep using the same ones we use for the fODF?

@arnaudbore
Copy link
Contributor

Conflict + I agree you should have specific parameters here.

@arnaudbore arnaudbore self-requested a review June 22, 2021 15:31
@ppoulin91
Copy link
Contributor Author

ppoulin91 commented Jun 22, 2021

I moved the SH fitting after pre-processing (following the same logic as DTI/FODF Metrics processes). Let me know if this is okay or if I should move stuff around/rename variables and processes.

@ppoulin91
Copy link
Contributor Author

Currently testing it on my remote machine, I'll let you know if everything runs smoothly

@ppoulin91
Copy link
Contributor Author

Fixed a typo, everything works on my end

Copy link
Contributor

@arnaudbore arnaudbore left a comment

Choose a reason for hiding this comment

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

Few typos otherwise LGTM.

USAGE Outdated
@@ -57,6 +57,14 @@ OPTIONAL BIDS ARGUMENTS (current value)

OPTIONAL ARGUMENTS (current value)

--sh_fitting Compute a Spherical Harmonics fitting onto the DWI, and output the SH coefficients in a Nifti file ($compute_sh).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--sh_fitting Compute a Spherical Harmonics fitting onto the DWI, and output the SH coefficients in a Nifti file ($compute_sh).
--sh_fitting Compute a Spherical Harmonics fitting onto the DWI, and output the SH coefficients in a Nifti file ($sh_fitting).

USAGE Outdated
-sh_order=6 for 28 directions
--sh_fitting_shell Shell selected to compute the SH fitting ($sh_fitting_shell).
NOTE: SH fitting works only on single shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

These options should not be at the top.

main.nf Outdated
export OMP_NUM_THREADS=1
export OPENBLAS_NUM_THREADS=1
scil_extract_dwi_shell.py $dwi \
$bval $bvec 0 $params.sh_fitting_shell ${sid}__dwi_sh_fittting.nii.gz \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$bval $bvec 0 $params.sh_fitting_shell ${sid}__dwi_sh_fittting.nii.gz \
$bval $bvec 0 $params.sh_fitting_shell ${sid}__dwi_sh_fitting.nii.gz \

main.nf Outdated
export OMP_NUM_THREADS=1
export OPENBLAS_NUM_THREADS=1
scil_extract_dwi_shell.py $dwi \
$bval $bvec 0 $params.sh_fitting_shell ${sid}__dwi_sh_fitting.nii.gz \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we must to use the same kind of thing that fodf_shells "0 1000".

Maybe we can raise an error when we select sh_fitting and don't set sh_fitting_shell. You have an example with fodf/dti_shells

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested it on my side and it works perfectly, I get all b0 + b1000 extracted. the script arg is set to nargs='+', so it automatically detects all b-vals. I thought the "quote" thing was specifically for nextflow because it couldn't handle multiple args

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay, I agree with @GuillaumeTh . you should have a warning when we select sh_fitting and don't set sh_fitting_shell and you should use the same kind of input as fodf_shells: "0 1000". Otherwise almost ready to be merged

@arnaudbore arnaudbore merged commit 8ab6197 into scilus:master Sep 7, 2021
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