CHrlS98 commented 4 months ago

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)
[x] 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

[x] My code follows the style guidelines of this project (run autopep8)
[x] I added relevant citations to scripts, modules and functions docstrings and descriptions
[x] I have performed a self-review of my code
[x] 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
[x] I moved all functions from the script file (except the argparser and main) to scilpy modules
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes

pep8speaks commented 4 months ago

Hello @CHrlS98, Thank you for updating !

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

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

CHrlS98 commented 4 months ago

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[bot] commented 4 months ago

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

```diff @@ 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](https://app.codecov.io/gh/scilus/scilpy/pull/925/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scilus) | Coverage Δ | | |---|---|---| | [Scripts](https://app.codecov.io/gh/scilus/scilpy/pull/925/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scilus) | `67.58% <80.00%> (-0.02%)` | :arrow_down: | | [Library](https://app.codecov.io/gh/scilus/scilpy/pull/925/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scilus) | `65.08% <41.96%> (-0.46%)` | :arrow_down: |

scilus / scilpy

Migrate code from aodf-toolkit #925