Adding SF amplitude thresholding as a way of create masks by frheault · Pull Request #1337 · scilus/scilpy

frheault · 2026-05-21T21:05:24Z

Quick description

New scripts that use FOD as input to create an improve WM mask based on either global or relative lobe amplitude to determine if the signal in a voxel is sufficient to be included in the mask.

This is also added to all 3 tracking algorithms to make it easier to use ''on-the-fly'', they are optional parameters (that I believe I will always use). It improves upon or even replace the need for an FA mask in some case.

There is still work to do (move duplicated code to a function for tracking), but I would like it if someone could test it on a random FOD file they have and see if the help/description/results are clear as it is a new behavior...

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

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

…t of StatefulImage)

…branch_b_independent

Copilot

Pull request overview

This PR introduces global spherical-function (SF) amplitude thresholding to generate white-matter–like masks from fODF/ODF inputs, and wires that capability into multiple tracking CLIs as optional “on-the-fly” masking.

Changes:

Added compute_max_sf_amplitude / compute_sf_threshold_mask utilities to derive per-voxel max SF amplitude and a global-threshold-based mask (with optional postprocessing).
Added new CLI scil_fodf_global_sf_threshold and registered it as a console script.
Added optional global SF thresholding arguments to tracking scripts/utilities and applied the resulting mask to tracking constraints.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`src/scilpy/tracking/utils.py`	Adds new tracking CLI options for global SF thresholding and adjusts `get_direction_getter` API to accept already-loaded data.
`src/scilpy/reconst/utils.py`	Adds core implementation for computing max SF amplitude and deriving a global-threshold SF mask.
`src/scilpy/cli/scil_tracking_pft.py`	Adds global SF thresholding options and applies resulting mask to include/exclude maps; adds sphere argument support.
`src/scilpy/cli/scil_tracking_local.py`	Applies optional global SF threshold mask during local tracking (CPU/GPU).
`src/scilpy/cli/scil_tracking_local_dev.py`	Applies optional global SF threshold mask when an ODF is provided; enforces argument validity.
`src/scilpy/cli/scil_fodf_global_sf_threshold.py`	New standalone CLI to compute and save a global SF threshold mask from SH or peaks input.
`pyproject.toml`	Registers the new CLI entrypoint.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-05-21T21:22:18Z

Codecov Report

❌ Patch coverage is 87.15084% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.59%. Comparing base (6952a18) to head (31323ad).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1337      +/-   ##
==========================================
+ Coverage   72.50%   72.59%   +0.09%     
==========================================
  Files         300      301       +1     
  Lines       26139    26276     +137     
  Branches     3671     3699      +28     
==========================================
+ Hits        18952    19076     +124     
- Misses       5640     5648       +8     
- Partials     1547     1552       +5

Flag	Coverage Δ
smoketests	`69.75% <76.53%> (+0.03%)`	⬆️
unittests	`14.21% <35.19%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Scripts	`75.33% <85.26%> (+0.05%)`	⬆️
Library	`69.31% <89.28%> (+0.14%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arnaudbore

LGTM: Add a test for one of the tractogram to use each param to get a better cover.

CHrlS98

Imports are in the middle of the code, is there a reason for this?

CHrlS98 · 2026-06-01T21:49:42Z

+    thr_g = p.add_mutually_exclusive_group(required=True)
+    thr_g.add_argument('--relative', type=float,
+                       help='Global SF threshold relative factor (0-1).')
+    thr_g.add_argument('--absolute', type=float,
+                       help='Global SF absolute threshold.')


does it need to be mutually exclusive? Could be both, no?

CHrlS98 · 2026-06-01T21:51:08Z

+    sf_mask = None
+    if args.global_sf_rel_thr is not None or \
+            args.global_sf_abs_thr is not None:
+        from scilpy.tracking.utils import get_global_sf_threshold_mask


any reason why this is a conditional import?

CHrlS98 · 2026-06-01T21:52:22Z

+
+        if args.global_sf_rel_thr is not None or \
+                args.global_sf_abs_thr is not None:
+            from scilpy.tracking.utils import get_global_sf_threshold_mask


again, why is this import inside the main?

CHrlS98 · 2026-06-01T21:55:09Z

+        else:
+            raise ValueError("Peaks input must be 4D or 5D.")
+
+        norms = np.linalg.norm(peaks, axis=-1)


I would consider adding a warning if all peaks have the same norm, meaning they'd be normalized.

CHrlS98 · 2026-06-01T21:55:45Z

+        mask = max_amp >= threshold
+
+    if postprocess_mask and np.any(mask):
+        import scipy.ndimage as ndi


Move import to top of file

CHrlS98 · 2026-06-01T21:57:02Z

+            for label, count in enumerate(label_counts_inverted):
+                if label == 0:
+                    continue  # Skip background
+                if count < 100:  # Threshold for filling holes


this feels a bit ad hoc no?

CHrlS98 · 2026-06-01T21:58:09Z

+                                    '    "1": {"propagator": "ODF", '
+                                    '"filename": str,\n'
+                                    '          "sh_basis": str, '
+                                    '"algo": str,\n'
+                                    '          "theta": float, '
+                                    '"step_size": float},\n'
+                                    '    "2": {"propagator": "ODF", '
+                                    '"filename": str,\n'
+                                    '          "sh_basis": str, '
+                                    '"algo": str,\n'
+                                    '          "theta": float, '
+                                    '"step_size": float}\n'


isn't this formatting very ugly?

CHrlS98 · 2026-06-01T21:59:57Z

+    return max_sf
+
+
+def compute_sf_threshold_mask(data, sphere_name='repulsion100',


given the postprocessing step this feels very tracking-oriented, not sure it's reconst.

CHrlS98

Alright thanks, I have one last request, but I'll approve right away bcause it's not breaking.

CHrlS98 · 2026-06-03T14:29:43Z

+            label_counts_inverted = np.bincount(labels_inverted.ravel())
+
+            # Fill holes smaller than 5% of the largest component size
+            hole_threshold = 0.05 * largest_component_size


My last request would be that this 0.05 is a parameter to the method, even if it is not exposed in the script.

arnaudbore

GTG

frheault added 5 commits May 19, 2026 22:23

Add global SF thresholding and advanced tracking features (Independen…

164922d

…t of StatefulImage)

Fix undefined variables and PEP8 cleanup for Branch B

11a7ed4

Merge branch 'master' of https://github.com/scilus/scilpy into split_…

496d68f

…branch_b_independent

arg parser fix

1172d7f

Merge after first split was merged

cd77e54

frheault requested a review from Copilot May 21, 2026 21:05

Copilot started reviewing on behalf of frheault May 21, 2026 21:05 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

Fix Copilot initial review and extracted redundant code

f33ee61

frheault changed the title ~~(WIP) Adding SF amplitude thresholding as a way of create masks~~ Adding SF amplitude thresholding as a way of create masks May 25, 2026

Improved descriptions, flake8 and utils harmo

172265a

arnaudbore previously approved these changes Jun 1, 2026

View reviewed changes

Comment thread src/scilpy/cli/scil_fodf_threshold_by_amplitude.py

arnaudbore requested a review from CHrlS98 June 1, 2026 20:18

CHrlS98 requested changes Jun 1, 2026

View reviewed changes

Fix Charles comments

5f87a6b

frheault dismissed arnaudbore’s stale review via 5f87a6b June 2, 2026 20:35

CHrlS98 previously approved these changes Jun 3, 2026

View reviewed changes

Switch 0.05 to a parameter

31323ad

frheault dismissed CHrlS98’s stale review via 31323ad June 4, 2026 12:34

arnaudbore approved these changes Jun 4, 2026

View reviewed changes

arnaudbore added this pull request to the merge queue Jun 4, 2026

Merged via the queue into scilus:master with commit b2bf4ac Jun 4, 2026
3 checks passed

		return max_sf


		def compute_sf_threshold_mask(data, sphere_name='repulsion100',

Conversation

frheault commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Quick description

Type of change

Provide data, screenshots, command line to test (if relevant)

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arnaudbore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CHrlS98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CHrlS98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaudbore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

frheault commented May 21, 2026 •

edited

Loading

codecov Bot commented May 21, 2026 •

edited

Loading