[FIX] allow full basis in accelerated tractography#40
[FIX] allow full basis in accelerated tractography#4036000 wants to merge 1 commit intodipy:masterfrom
Conversation
|
@claude can you translate these changes to webgpu and metal? |
|
Superceded by #41 |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix CUDA tractography behavior for asymmetric (full spherical-harmonic basis) ODFs by avoiding antipodal symmetry assumptions during direction filtering / peak de-duplication.
Changes:
- Add a
full_basisflag to the CUDAGPUTrackerand plumb it into NVRTC compile-time macros. - Introduce
APPLY_ABS_IF_SYM()in CUDA headers and use it where dot-products were previously wrapped inFABS(). - Update CUDA kernels to conditionally apply absolute value to dot-product similarity checks based on
FULL_BASIS.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cuslines/cuda_python/cu_tractography.py |
Adds full_basis parameter to CUDA GPUTracker and stores it on the instance. |
cuslines/cuda_python/cu_direction_getters.py |
Passes FULL_BASIS as a compile-time macro into the NVRTC build. |
cuslines/cuda_c/globals.h |
Defines APPLY_ABS_IF_SYM() based on FULL_BASIS. |
cuslines/cuda_c/tracking_helpers.cu |
Uses APPLY_ABS_IF_SYM() when removing similar peak directions. |
cuslines/cuda_c/generate_streamlines_cuda.cu |
Uses APPLY_ABS_IF_SYM() when filtering candidate directions by angular similarity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sphere_vertices: np.ndarray, | ||
| sphere_edges: np.ndarray, | ||
| full_basis: bool = False, | ||
| max_angle: float = radians(60), | ||
| step_size: float = 0.5, |
There was a problem hiding this comment.
Adding full_basis in the middle of the GPUTracker.__init__ positional parameters is a backward-incompatible API change: any downstream code calling GPUTracker(..., sphere_edges, max_angle, step_size, ...) positionally will silently shift arguments (e.g., max_angle becomes full_basis=True), leading to incorrect tracking without an obvious error. To preserve compatibility, move full_basis to the end of the parameter list or make it keyword-only (e.g., introduce a * before full_basis).
| sphere_vertices: np.ndarray, | ||
| sphere_edges: np.ndarray, | ||
| full_basis: bool = False, | ||
| max_angle: float = radians(60), | ||
| step_size: float = 0.5, |
There was a problem hiding this comment.
This adds full_basis only to the CUDA backend GPUTracker constructor, which makes the top-level cuslines.GPUTracker API backend-dependent (Metal/WebGPU trackers do not accept this argument). The repo docs state the three backends "share the same API surface", so this introduces an inconsistency for backend-agnostic callers. Consider adding the same full_basis parameter (even if currently ignored) to MetalGPUTracker and WebGPUTracker, and ideally propagate it into their shader code paths as well, or otherwise document/encapsulate the divergence.
This fixes a bug and actually allows GPU accelerated tractography to take advantage of asymmetric ODFs.