Clarify b0 threshold

[EmmaRenauld]

Quick description

1. [ENH] The argument --force_b0_threshold, used a bit everywhere, was quite unclear. Does it force to use the b0_threshold given, or does it force to work in any case? Answer was: it forced to use the minimal b-value even if it was higher than expected b0_threshold. Renamed --skip_b0_validation.

2. [FIX] In many cases, the function check_b0_threshold was incorrectly used in the scripts (they were using check_b0_threshold(args) but the function expected a bool as first argument. It was thus not doing as expected.

3. [FIX] In other cases, it was used with: b0_thr = check_b0_threshold(force_b0_threshold, bvals.min(), bvals.min()) which was not doing anything!

4. [ENH] It was always used with the default b0_threshold. Added an argument b0_threshold that user may change if there is an error.

5. [FIX] Major fix in scil_sh_to_sf. With option --in_bval, The minimal b-value was always considered as a b0 and rejected from the average, even if the --in_bval contained no b0, which is actually kind of expected in this particular script.

Complex description :)

Easy scripts:

• scil_dwi_detect_volume_outliers: Ok. Usage was already clean. Just using the new official arg.
• scil_dwi_extract_b0: Idem.
• scil_dwi_to_sh: Ok. Idem. However, arg --force_b0 existed but was not used anywhere! Fixed.

More complicated ones:

• scil_fodf_ssst: The usage of a b0_threshold seems straightforward for me but was not implemented in this script! By using check_b0_threshold(args.force_b0_threshold, bvals.min(), bvals.min()), it was always setting the b0_thr to the minimal b-value! But dipy's csdeconv does seem to use the gtab.b0s_mask. Added the b0_threshold option.
• scil_sh_to_sf: There was no option. Used the minimal b-value as threshold. Option added.
• scil_qball_metrics: Option existed, but then gtab was created with bvals.min(). Fixed, and added option b0_threshold.
• scil_frf_ssst: The usage of a b0_threshold seems straightforward for me but was not implemented in this script! Added the b0_threshold option.

Impossible ones for now:

• scil_dti_metrics: I added some text. The gtab.b0_masks does not seem used anywhere in dipy's tensor model I think. To be confirmed by some expert.
• scil_fodf_msmt: Cannot manage a separate threshold right now. Added link to dipy's issue. Added explanation. Using tolerance as b0_threshold.
• scil_dki_metrics: Could not modify the identify_shells method right now. Only using tolerance. Link to dipy's issue also added. But if you think usage is clear in dki, please tell me what it does, I will modify the text.
• scil_frf_msmt: Could not modify the extract_dwi_shell method right now. Only using tolerance.
• scil_frf_memsmt: Could not modify the genereate_btensor_input method right now. Only using tolerance. No checks at all here.
• scil_fodf_memsmt: Idem.
• scil_btensor_metrics: Idem. Removed unused option force_b0.

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)

...

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
• [ x] I have made corresponding changes to the documentation
• [ x] 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

[EmmaRenauld]

See associated Issue #866

[pep8speaks]

Hello @EmmaRenauld, Thank you for updating !

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

Comment last updated at 2024-02-29 18:49:17 UTC

[arnaudbore]

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-867/1/

[arnaudbore]

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-867/2/

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-867/7/

[arnaudbore]

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-867/8/

[EmmaRenauld]

Should be ready for second review, @arnaudbore , @karanphil
Hoping we will not change our minds again!

Adding here reference to dipy issue: dipy/dipy#3015

[arnaudbore]

Build passed ! Good Job 🍻 !

[karanphil]

Good job! I've got a few comments, but after they are adressed I think we are gtg.

[arnaudbore]

A couple of things but we are getting there !

[arnaudbore]

Build passed ! Good Job 🍻 !

[codecov]

Codecov Report

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

Project coverage is 69.27%. Comparing base (4c2848c) to head (3ce0db0).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
+ Coverage   69.17%   69.27%   +0.09%     
==========================================
  Files         389      389              
  Lines       20963    20980      +17     
  Branches     3233     3239       +6     
==========================================
+ Hits        14501    14533      +32     
+ Misses       5132     5122      -10     
+ Partials     1330     1325       -5     

Components	Coverage Δ
Scripts	71.70% <96.73%> (+0.10%)	⬆️
Library	65.14% <95.83%> (+0.08%)	⬆️

[mdesco]

Looks good to me. Did not test every script. Tested one and read all the --help.

Massive PR. Nice job!