Migrate code from aodf-toolkit

[CHrlS98]

Quick description

This PR is a replacement for pull request #722 . Migrate code from https://github.com/CHrlS98/aodf-toolkit. Changes mostly apply to script scil_sh_to_aodf.py and replaces the bilateral method by the unifiedmethod implemented in aodf-toolkit and described in https://doi.org/10.1016/j.neuroimage.2024.120516. I also added a 100% python implementation of the method which I plan to add in DIPY at some point.

As you may know, opencl programs can not only run on GPU hardware, but also on CPU. So now there is an --use_opencl for using OpenCL as well as an option --device allowing you to choose between cpu and gpu. I don't know if it is the best way to do it as running on GPU requires that --use_opencl and --device gpu are manually set. By default the code will run on the CPU and won't use OpenCL.

I also updated the opencl_utils module and modified the GPUTracker to work with the new interface.

Type of change

Check the relevant options.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

# run on the cpu using a python implementation by default 
# (this is long on a whole brain but takes 1-2 minutes on the fibercup)
scil_sh_to_aodf.py fodf.nii.gz afodf.nii.gz -v

# to run on the gpu
scil_sh_to_aodf.py fodf.nii.gz afodf.nii.gz -v --use_opencl --device gpu

Checklist

• My code follows the style guidelines of this project (run autopep8)
• I added relevant citations to scripts, modules and functions docstrings and descriptions
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I moved all functions from the script file (except the argparser and main) to scilpy modules
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[pep8speaks]

Hello @CHrlS98, Thank you for updating !

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-03-13 18:01:21 UTC

[CHrlS98]

There was a bug in the previous bilateral filter implementation, rendering test data unsuitable for testing the fixed script. I fixed the tests, they run locally. Here is the new test data: fodf_filtering.zip.

[codecov]

Codecov Report

Attention: Patch coverage is 45.78313% with 135 lines in your changes are missing coverage. Please review.

Project coverage is 66.63%. Comparing base (76b35de) to head (a252b27).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #925      +/-   ##
==========================================
- Coverage   66.82%   66.63%   -0.20%     
==========================================
  Files         392      392              
  Lines       20977    21094     +117     
  Branches     3207     3237      +30     
==========================================
+ Hits        14017    14055      +38     
- Misses       5658     5719      +61     
- Partials     1302     1320      +18     

Components	Coverage Δ
Scripts	67.58% <80.00%> (-0.02%)	⬇️
Library	65.08% <41.96%> (-0.46%)	⬇️

[arnaudbore]

LGTM

[karanphil]

I want very quickly over the bits of code I understood. I had e few questions but outside of that it looks good from my very limited knowledge of the subject.

[EmmaRenauld]

Like Phil, I can't really read the science, nor the .cl files. But everything seams clear and well documented. I didn't see any obvious bug. Some small suggestions.

[EmmaRenauld]

Arnaud, should this kind of small method go in, say, reconst.utils or something like this? Or, considering that it's only used here, keeping it here with a _ is fine?

[EmmaRenauld]

Is this the same data as in the verifications in scil_sh_to_aodf? Are you doing twice the same test, once as unit test and once in the scripts tests? If so, the unit test could be removed if you wish, and you just add a comment to see the script test. If different, could you still add a comment in the unit test to reference to the script test and explain the difference?

[CHrlS98]

this data is only used in the unit tests while the script test uses a nifti image and tests the script input/output. I'll add a note

[EmmaRenauld]

I meant: did you open the nifti to see its values and copy them here? Or they are entirely other values?

[CHrlS98]

Entirely other values, they are from the fibercup while the nifti is a whole brain dataset.