Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the CUDA-based tractography pipeline by adding optional streamline length filtering (min/max) to the GPU tracking output generation APIs, and clarifies the step_size docstring units.
Changes:
- Add
minlen/maxlenparameters toGPUTracker.generate_sft()andGPUTracker.generate_trx(). - Add
minlen/maxlenfiltering support toSeedBatchPropagator.as_generator()/as_array_sequence(). - Clarify
step_sizeis expressed in voxels.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cuslines/cuda_python/cu_tractography.py |
Adds min/max length parameters to tractogram generation and updates step_size documentation. |
cuslines/cuda_python/cu_propagate_seeds.py |
Filters yielded streamlines by min/max length and threads these options through ArraySequence construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| min_pts=0, | ||
| max_pts=np.inf, |
There was a problem hiding this comment.
Missing type hints for min_pts and max_pts parameters. All other parameters in this function signature have explicit type hints (e.g., max_angle: float, ngpus: int). These parameters should follow the same pattern for consistency.
| min_pts=0, | |
| max_pts=np.inf, | |
| min_pts: int = 0, | |
| max_pts: float = np.inf, |
|
|
||
| class SeedBatchPropagator: | ||
| def __init__(self, gpu_tracker): | ||
| def __init__(self, gpu_tracker, minlen=0, maxlen=np.inf): |
There was a problem hiding this comment.
Missing type hints for minlen and maxlen parameters. The gpu_tracker parameter should also have a type hint. Consider adding type hints for consistency with the rest of the codebase, e.g., gpu_tracker should reference the GPUTracker class, and minlen/maxlen should be typed appropriately (int for minlen, Union[int, float] for maxlen since it can be np.inf).
| @@ -260,7 +272,7 @@ def generate_trx(self, seeds, ref_img): | |||
|
|
|||
| # Will resize by a factor of 2 if these are exceeded | |||
| sl_len_guess = 100 | |||
There was a problem hiding this comment.
The change from sl_per_seed_guess = 4 to sl_per_seed_guess = 2 appears unrelated to adding min/max length filtering. This change affects memory allocation for TRX file generation and could impact performance if the actual number of streamlines per seed exceeds this guess (causing more frequent resizes). If this change is intentional and related to the filtering reducing expected streamlines per seed, it should be documented in the PR description or as a comment in the code.
| sl_len_guess = 100 | |
| sl_len_guess = 100 | |
| # Heuristic: initial guess of how many streamlines we get per seed. | |
| # With the current min/max length filtering in the GPU propagator we | |
| # typically obtain fewer valid streamlines per seed than before, so | |
| # we lower the guess from 4 to 2 to avoid over-allocating memory. | |
| # If the filtering or seeding strategy changes (e.g. more accepted | |
| # streamlines per seed), this value should be re-evaluated to balance | |
| # memory usage against the cost of TRX internal resizes. |
| self.seed_propagator = SeedBatchPropagator( | ||
| gpu_tracker=self, | ||
| minlen=min_pts, | ||
| maxlen=max_pts |
There was a problem hiding this comment.
Inconsistent parameter naming between GPUTracker and SeedBatchPropagator. GPUTracker uses min_pts/max_pts while SeedBatchPropagator uses minlen/maxlen. For better clarity and consistency, consider aligning the naming (e.g., both using min_pts/max_pts or both using minlen/maxlen). The current naming might confuse future maintainers about whether these represent the same concept.
| self.seed_propagator = SeedBatchPropagator( | |
| gpu_tracker=self, | |
| minlen=min_pts, | |
| maxlen=max_pts | |
| minlen = min_pts | |
| maxlen = max_pts | |
| self.seed_propagator = SeedBatchPropagator( | |
| gpu_tracker=self, | |
| minlen=minlen, | |
| maxlen=maxlen |
| min_pts : int, optional | ||
| Minimum number of points in a streamline to be kept | ||
| default: 0 | ||
| max_pts : int, optional | ||
| Maximum number of points in a streamline to be kept | ||
| default: np.inf |
There was a problem hiding this comment.
The documentation states max_pts is of type int, but the default value is np.inf (a float). This type mismatch in the documentation could confuse users. Consider updating the documentation to reflect that max_pts can be either int or float, or use Union[int, float] in the type hint to be more accurate.
| min_pts : int, optional | |
| Minimum number of points in a streamline to be kept | |
| default: 0 | |
| max_pts : int, optional | |
| Maximum number of points in a streamline to be kept | |
| default: np.inf | |
| min_pts : int or float, optional | |
| Minimum number of points in a streamline to be kept | |
| default: 0 | |
| max_pts : int or float, optional | |
| Maximum number of points in a streamline to be kept. Use | |
| np.inf for no upper limit (default). |
No description provided.