karanphil commented 4 months ago

Quick description

I added the legacy option for SH basis to all scripts, except scil_qball_metrics.py which do not have the legacy support from Dipy. I did a PR in Dipy, but in the meantime we could convert the SH basis on our end, or just not support anything else than legacy=True (the default in Dipy) for this script. In summary, the peaks_from_model function from dipy.direction.peaks does not have a legacy option for the moment, so we are stuck with the default legacy option (True) for scil_qball_metrics.py.

This resulted in the creation of an interpret_sh_basis function to io.utils, which extracts the SH basis name and the legacy option from the sh_basis argument. The scil_sh_convert.py script changed the most, as it now requires two choices from the sh_basis argument.

...

Type of change

Check the relevant options.

[ ] Bug fix (non-breaking change which fixes an issue)
[x] 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)

...

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

Hello @karanphil, Thank you for updating !

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

Comment last updated at 2024-03-01 01:16:16 UTC

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 69.37%. Comparing base (1c3029a) to head (1bb9898).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #921 +/- ## ========================================== + Coverage 69.27% 69.37% +0.10% ========================================== Files 389 389 Lines 20980 20997 +17 Branches 3239 3231 -8 ========================================== + Hits 14533 14566 +33 + Misses 5122 5115 -7 + Partials 1325 1316 -9 ``` | [Components](https://app.codecov.io/gh/scilus/scilpy/pull/921/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/921/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scilus) | `71.79% <97.22%> (+0.09%)` | :arrow_up: | | [Library](https://app.codecov.io/gh/scilus/scilpy/pull/921/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scilus) | `65.25% <95.23%> (+0.11%)` | :arrow_up: |

scilus / scilpy