Add SH basis legacy support #921
Conversation
|
Hello @karanphil, Thank you for updating ! There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-03-01 01:16:16 UTC |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
|
| is_legacy : bool | ||
| Whether or not the SH basis is in its legacy form. | ||
| """ | ||
| if len(args.sh_basis) == 2: |
There was a problem hiding this comment.
it works but you could also simply do
basis = 'descoteaux07' if 'descoteaux07' in sh_basis_name else 'tournier07'
legacy = 'legacy' in sh_basis_name
There was a problem hiding this comment.
Yes but I prefer the more "general" way, without assuming descoteaux07 or tournier07. @arnaudbore ? I think Charles refers to lines 313 to 316 and 319 to 320.
There was a problem hiding this comment.
I would agree with you if we were not using choices in add_sh_basis_args where we are already assuming descoteaux07 and tournier07. I would go for @CHrlS98 suggestion more elegant 👍
There was a problem hiding this comment.
Is it better now?
scilpy/reconst/utils.py
Outdated
| @@ -46,12 +46,14 @@ def _honor_authorsnames_sh_basis(sh_basis_type): | |||
| return sh_basis | |||
|
|
|||
|
|
|||
| def get_b_matrix(order, sphere, sh_basis_type, return_all=False): | |||
| def get_b_matrix(order, sphere, sh_basis_type, return_all=False, | |||
There was a problem hiding this comment.
I do not think we actually need this function anymore, it just calls DIPY's method, no?
There was a problem hiding this comment.
I thought the same thing... @arnaudbore ?
There was a problem hiding this comment.
Can you show me what it will look like ? It seems that get_b_matrix is doing a little bit more than one call to DIPY's method.
There was a problem hiding this comment.
A couple of things to correct related to @CHrlS98 comments apart from that almost LGTM 😃
| is_legacy : bool | ||
| Whether or not the SH basis is in its legacy form. | ||
| """ | ||
| if len(args.sh_basis) == 2: |
There was a problem hiding this comment.
I would agree with you if we were not using choices in add_sh_basis_args where we are already assuming descoteaux07 and tournier07. I would go for @CHrlS98 suggestion more elegant 👍
scilpy/reconst/utils.py
Outdated
| @@ -46,12 +46,14 @@ def _honor_authorsnames_sh_basis(sh_basis_type): | |||
| return sh_basis | |||
|
|
|||
|
|
|||
| def get_b_matrix(order, sphere, sh_basis_type, return_all=False): | |||
| def get_b_matrix(order, sphere, sh_basis_type, return_all=False, | |||
There was a problem hiding this comment.
Can you show me what it will look like ? It seems that get_b_matrix is doing a little bit more than one call to DIPY's method.
| is_legacy : bool | ||
| Whether or not the SH basis is in its legacy form. | ||
| """ | ||
| if len(args.sh_basis) == 2: |
Quick description
I added the legacy option for SH basis to all scripts, except
scil_qball_metrics.pywhich 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, thepeaks_from_modelfunction fromdipy.direction.peaksdoes not have a legacy option for the moment, so we are stuck with the default legacy option (True) forscil_qball_metrics.py.This resulted in the creation of an
interpret_sh_basisfunction toio.utils, which extracts the SH basis name and the legacy option from the sh_basis argument. Thescil_sh_convert.pyscript changed the most, as it now requires two choices from the sh_basis argument....
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist