Skip to content

[FIX] Add NUMBA backend; correctly handle symmetric/nonsymmetric spheres#41

Open
36000 wants to merge 9 commits intomasterfrom
allow_full_basis
Open

[FIX] Add NUMBA backend; correctly handle symmetric/nonsymmetric spheres#41
36000 wants to merge 9 commits intomasterfrom
allow_full_basis

Conversation

@36000
Copy link
Copy Markdown
Collaborator

@36000 36000 commented Apr 28, 2026

This fixes a bug and actually allows GPU accelerated tractography to take advantage of asymmetric ODFs.

Copilot AI review requested due to automatic review settings April 28, 2026 18:03
@36000
Copy link
Copy Markdown
Collaborator Author

36000 commented Apr 28, 2026

@copilot can you translate these changes to webgpu and metal?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes GPU-accelerated tractography behavior for asymmetric ODFs by making “antipodal symmetry” (using abs(dot)) conditional, so full-basis spherical harmonics can preserve directionality.

Changes:

  • Added a full_basis option to GPUTracker and propagated it into NVRTC compile-time macros.
  • Introduced APPLY_ABS_IF_SYM() in CUDA headers to toggle absolute-value behavior based on FULL_BASIS.
  • Updated CUDA direction similarity checks to use the new conditional macro.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cuslines/cuda_python/cu_tractography.py Adds full_basis parameter to GPUTracker and stores it for compilation.
cuslines/cuda_python/cu_direction_getters.py Passes FULL_BASIS macro into runtime CUDA compilation.
cuslines/cuda_c/globals.h Defines APPLY_ABS_IF_SYM() based on FULL_BASIS.
cuslines/cuda_c/tracking_helpers.cu Uses conditional abs in peak de-duplication similarity check.
cuslines/cuda_c/generate_streamlines_cuda.cu Uses conditional abs in direction filtering against MAX_ANGLE.

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

Comment thread cuslines/cuda_python/cu_tractography.py
Copy link
Copy Markdown

Copilot AI commented Apr 28, 2026

@copilot can you translate these changes to webgpu and metal?

Done in commit f245353. Here's what was translated to each backend:

Metal (cuslines/metal_shaders/globals.h, generate_streamlines_metal.metal, tracking_helpers.metal, mt_tractography.py, mt_direction_getters.py):

  • Added APPLY_ABS_IF_SYM macro to globals.h (controlled by #define FULL_BASIS, prepended at compile time)
  • Applied it in the angle-filter loop (generate_streamlines_metal.metal) and the similar-vertex-removal loop (tracking_helpers.metal), mirroring the CUDA changes exactly
  • MetalGPUTracker now accepts full_basis=False and passes it to setup_device / compile_program

WebGPU (cuslines/wgsl_shaders/generate_streamlines.wgsl, tracking_helpers.wgsl, wg_tractography.py, wg_direction_getters.py):

  • Since WGSL has no preprocessor, compile_program prepends const FULL_BASIS: u32 = X; to the concatenated source
  • Uses WGSL's select(abs(dot_val), dot_val, FULL_BASIS == 1u) at the two equivalent sites
  • WebGPUTracker now accepts full_basis=False and propagates it through setup_device / compile_program

@36000
Copy link
Copy Markdown
Collaborator Author

36000 commented Apr 28, 2026

@neurolabusc this update may be of interest ot you. it fixes a bug with probabilistic tractography using asymmetric ODFs. I had copilot also implement the changes in the webgpu and metal versions

@36000 36000 changed the title [FIX] allow full basis in accelerated tractography [FIX] Add NUMBA backend; allow full basis in accelerated tractography May 2, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

pyproject.toml:48

  • Adding numba as a required dependency conflicts with requires-python = ">=3.7" (Numba has not supported Python 3.7 for several releases) and with the runtime messaging that suggests a cuslines[numba] extra. Either (1) bump requires-python to a Numba-supported version, or (2) move numba into [project.optional-dependencies] as a numba extra and keep the core install Python-3.7-compatible.
requires-python = ">=3.7"
dependencies = [
    "numpy",
    "numba",
    "nibabel",
    "tqdm",
    "dipy",
    "trx-python",
    "scipy",
]

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

Comment thread cuslines/numba/nu_tractography.py Outdated
Comment thread cuslines/__init__.py Outdated
Comment thread cuslines/__init__.py Outdated
Comment thread cuslines/generic_tracker.py
Comment thread run_gpu_streamlines.py
Comment thread run_gpu_streamlines.py Outdated
Comment thread cuslines/numba_njit/generate_streamlines_numba.py
Comment thread run_gpu_streamlines.py Outdated
Comment thread run_gpu_streamlines.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

run_gpu_streamlines.py:439

  • The PR introduces a full_basis path, but run_gpu_streamlines.py doesn’t expose any way to enable it and also doesn’t pass full_basis into Tracker(...). As a result, the new asymmetric-ODF behavior is effectively unreachable from this script. Consider adding a --full-basis CLI flag (and wiring it into both the DIPY SH/ODF construction and the Tracker(full_basis=...) argument) so users can actually run the intended mode end-to-end.
    with Tracker(
        dg,
        data,
        FA,
        args.fa_threshold,
        sphere.vertices,
        sphere.edges,
        max_angle=args.max_angle * np.pi / 180,
        step_size=args.step_size,
        relative_peak_thresh=args.relative_peak_threshold,
        min_separation_angle=args.min_separation_angle * np.pi / 180,
        ngpus=args.ngpus,
        rng_seed=0,
        chunk_size=args.chunk_size,
    ) as gpu_tracker:

cuslines/init.py:88

  • Top-level cuslines now exports Tracker but no longer exports the historical GPUTracker name. This is a breaking public API change for any downstream code doing from cuslines import GPUTracker. Consider keeping a backwards-compatible alias (e.g., GPUTracker = Tracker) and including it in __all__ while still preferring Tracker going forward.
__all__ = [
    "Tracker",
    "ProbDirectionGetter",
    "PttDirectionGetter",
    "BootDirectionGetter",
    "BACKEND",
]

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

Comment thread cuslines/numba/nu_tractography.py Outdated
Comment thread cuslines/numba/nu_tractography.py Outdated
Comment thread pyproject.toml
Comment thread cuslines/generic_tracker.py
Comment thread cuslines/numba_njit/generate_streamlines_numba.py
@36000 36000 changed the title [FIX] Add NUMBA backend; allow full basis in accelerated tractography [FIX] Add NUMBA backend; correctly handle symmetric/nonsymmetric spheres May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants