Tractogram scripts parts4

[EmmaRenauld]

Quick description

Modified the tractogram filtering scripts! Looks like a lot of changes but the core methods should not have changed. Only the managing of all inputs and options.

• scil_tractogram_filter_by_length: ok. Cite other filtering scripts.
• scil_tractogram_filter_by_orientation: ok. Cite other filtering scripts.
• scil_tractogram_filter_by_anatomy: Will be difficult to review, sorry!
  • Clarified doc.
  • There was a lot of repeated code. Merged into _finalize_step or into _finish_all.
  • Moved method binarize_labels to image.labels.get_binary_mask_from_labels
  • Removed method dilate_mask. Now uses scipy, like we do in scil_volume_math.py! Dilation ratio now in voxels. Confirmed by @frheault
  • Method compute_outliersis not required anymore; I made sure that all filtering methods used also return the outlier streamlines or their ids.
  • Other methods (save_intermediate_sft, save_rejected, display_count, save_count) are now all included in _finalize_step.
• scil_tractogram_filter_by_roi: Will be difficult to review, sorry!
  • Clarified doc.
  • Removed the explanation that conditions will be applied sequentially. Not entirely true. We get the argparser's --drawn_roi and --atlas_roi separately, for instance. To do it sequentially, we would need to get the input as is and parse it ourselves. However, I did change the management of the filtering_list option. Before, it was indeed sequential only for the filtering_list options! (and other options such as --drawn_roi were done before which was not clear from explanation!!). I can revert my changes if you want, to keep sequential management. But then, I would explain better in the doc that it's only for filtering_list, and I would prevent using filtering_list with any other ROI option. Any comment? @frheault
  • Moved stuff around to answer the "toDo" that was written: # Todo. Prepare now the names of other files (ex, ROI) and verify if exist and compatible.
  • Question: Why is there an option --overwrite_distance?? Just weird for me. User could just call it correctly the first time! Still used? @frheault

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

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 @EmmaRenauld, Thank you for updating !

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

Comment last updated at 2024-05-14 15:09:27 UTC

[codecov]

Codecov Report

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

Project coverage is 68.37%. Comparing base (11f218b) to head (fbd68fb).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
+ Coverage   67.97%   68.37%   +0.40%     
==========================================
  Files         419      419              
  Lines       21615    21544      -71     
  Branches     3251     3219      -32     
==========================================
+ Hits        14693    14731      +38     
+ Misses       5633     5535      -98     
+ Partials     1289     1278      -11     

Components	Coverage Δ
Scripts	69.51% <68.62%> (+0.55%)	⬆️
Library	66.68% <88.46%> (+0.18%)	⬆️

[CHrlS98]

Minor comments

[arnaudbore]

Seems GTG, two huge scripts I would need to compare before merging.

[EmmaRenauld]

Writing here, so that we don't forget:
As discovered by @arnaudbore, the new dilation usage creates a difference in the results are compared to the master branch.
gm_mask on master: 839 238 voxels.
gm_mask here: 854 041 voxels.

[EmmaRenauld]

Writing here, so that we don't forget: As discovered by @arnaudbore, the new dilation usage creates a difference in the results are compared to the master branch. gm_mask on master: 839 238 voxels. gm_mask here: 854 041 voxels.

The end of the discussion was:

Should we modify the option in the script to have a --dilate_radius in number of voxel (= number of pass) like in scil_volume_math, or do we need to keep it in mm?

[frheault]

About your questions for the 'sequential' nature of scil_tractogram_filter_with_roi.py. I think it is not actually so important to mention if it is sequential or not since we only support logical AND so the results are the same.