CUDA support via the Adapt mechanism by RainerHeintzmann · Pull Request #56 · bionanoimaging/FourierTools.jl

RainerHeintzmann · 2025-03-22T20:01:45Z

This is another fresh attempt to gain CUDA support.
It needs the very recent version of NDTools.jl to work (using the select_region_view() function).
Also the classes internal to FourierTools.jl have been extended via the Adapt mechanism.
The direction paramater was also moved to a Val() type.

The CircShiftedArray.jl was copied from ShiftedArrays.jl and modified a little by also including it into the Adapt mechanism which is based on type Unions. It might be preferable to rather adapt the original implementation.

codecov · 2025-04-05T11:37:09Z

Codecov Report

Attention: Patch coverage is 69.68085% with 57 lines in your changes missing coverage. Please review.

Project coverage is 89.98%. Comparing base (60de955) to head (5220a1f).

Files with missing lines	Patch %	Lines
ext/CUDASupportExt_FT.jl	25.00%	30 Missing ⚠️
ext/CUDASupportExt_SA.jl	28.57%	25 Missing ⚠️
src/custom_fourier_types.jl	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   94.03%   89.98%   -4.05%     
==========================================
  Files          19       21       +2     
  Lines        1173     1278     +105     
==========================================
+ Hits         1103     1150      +47     
- Misses         70      128      +58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

roflmaostc · 2025-06-30T09:09:21Z

+    CUDA.CuArrayStyle{N,CD}()
+end
+
+function Base.copy(s::AllShiftedAndViews)


this is type piracy?

Because you define a copy method from Base with types owned by base. So this should not be done.

I don't think this is type piracy here. AllShiftedAndViews only contains types or unions which are defined in this package. However there are arguments one could make with the CUDASupportExt_SA.jl. In that case, I really don't know what to do, since the author of ShiftedArrays.jl is non-responsive.
JuliaArrays/ShiftedArrays.jl#70
JuliaArrays/ShiftedArrays.jl#64
Maybe I just dublicate the code for the circshiftedArray.jl into MutableShiftedArrays.jl and host the extension there and then remove the dependence on ShiftedArrays.jl entirely.?

I added the necessary code to MutableShiftedArrays.jl and opened a PR there:
RainerHeintzmann/MutableShiftedArrays.jl#2
Once this is merged and a new version released, one can remove the dependency on ShiftedArrays.jl entirely.

roflmaostc · 2025-06-30T09:11:44Z

I think the main issue is that you define methods such as Base.copy with types from base SubArray. This is type piracy and should be avoided at all costs.

roflmaostc · 2025-08-04T09:14:58Z

+
+# Define the BroadcastStyle for SubArray of MutableShiftedArray with CuArray
+
+function Base.Broadcast.BroadcastStyle(::Type{T})  where {N, CD, T<:AllSubArrayTypeCu{N, CD}}


this is type piracy. Because you use Base types and a base method.

The plan is to entirely remove CUDASupportExt_SA.jl since this is then covered in MutableShiftedArrays.jl.

did you remove it?

See above "The plan is". Once I merged the other package, I can simply remove it here. The decision to be made: A) have circshifted array exist in ShiftedArrays.jl and MutableShiftedArrays.jl with currently identical code, or B) rename CircShiftedArray to MutableCircShiftedArray with identical functionality and currently identical code (except for the CUDA support extension. Or C) import ShiftedArrays.jl in MutableShiftedArrays.jl and reexport with the extension (I think this would still be type piracy). My preference: option A). What is yours?

roflmaostc · 2025-08-04T09:16:13Z

+    # tmp = similar(mat, (8, 10))
+    # tmp .= 1
+    # return rft_fix_after(tmp, old_size, new_size)


can be deleted?

roflmaostc · 2025-08-04T09:16:23Z

+# """
+#     select_region(mat; new_size)
+
+# performs the necessary Fourier-space operations of resampling
+# in the space of ft (meaning the already circshifted version of fft).
+
+# `new_size`.
+# The size of the array view after the operation finished. 
+
+# `center`.
+# Specifies the center of the new view in coordinates of the old view. By default an alignment of the Fourier-centers is assumed.
+# # Examples
+# ```jldoctest
+# julia> using FFTW, FourierTools
+
+# julia> select_region(ones(3,3),new_size=(7,7),center=(1,3))
+# 7×7 PaddedView(0.0, OffsetArray(::Matrix{Float64}, 4:6, 2:4), (Base.OneTo(7), Base.OneTo(7))) with eltype Float64:
+#  0.0  0.0  0.0  0.0  0.0  0.0  0.0
+#  0.0  0.0  0.0  0.0  0.0  0.0  0.0
+#  0.0  0.0  0.0  0.0  0.0  0.0  0.0
+#  0.0  1.0  1.0  1.0  0.0  0.0  0.0
+#  0.0  1.0  1.0  1.0  0.0  0.0  0.0
+#  0.0  1.0  1.0  1.0  0.0  0.0  0.0
+#  0.0  0.0  0.0  0.0  0.0  0.0  0.0
+# ```
+# """
+# function select_region(mat; new_size=size(mat), center=ft_center_diff(size(mat)).+1, pad_value=zero(eltype(mat)))
+#     new_size = Tuple(expand_size(new_size, size(mat)))
+#     center = Tuple(expand_size(center, ft_center_diff(size(mat)) .+ 1))
+#     oldcenter = ft_center_diff(new_size) .+ 1
+#     PaddedView(pad_value, mat, new_size, oldcenter .- center.+1);
+# end


can be deleted?

roflmaostc · 2025-08-04T09:17:12Z

        end
-        arr = select_region(arr, new_size=old_size .+ extra_size, pad_value=pad_value)
+
+        arr = select_region_view(arr, new_size=old_size .+ extra_size, pad_value=pad_value)


why is this changed to the view?

... to avoid copying too much and to reduce memory requirements.

did you benchmark? Did it change anything?
I would not change any unaffected code by this introduction.

roflmaostc · 2025-08-04T09:18:02Z

+```jldoctest
+julia> FourierTools.bc_size(rand(5, 2, 3), rand(1, 2))
+(5, 2, 3)
+"""


missing ```

roflmaostc · 2025-08-04T09:18:36Z

 end

+"""
+    get_idxrng_around_center(arr_1, arr_2)


what is idxrng?

See the documentation. idxrng stands for "index_range".

maybe give it the verbose name then. it's not exported anyway

roflmaostc · 2025-08-04T09:18:44Z

+    # out_is = []
+    # for i = 1:ndims(arr_large)
+    #     a, b = get_indices_around_center(size(arr_large)[i], size(arr_small)[i])
+    #     push!(out_is, a:b)
+    # end



roflmaostc · 2025-08-04T09:19:09Z

    N = 5
-    psf = abs.(randn((N, N, 2)))
-    img = randn((N, N, 2))
+    psf = opt_cu(abs.(randn((N, N, 2))), use_cuda)


what is opt_cu?

This is the mechanism used in the tests to locally test with CUDA but upon deployment test without CUDA. It is defined at the beginning of runtests.jl.

roflmaostc · 2025-08-08T12:46:46Z

We need tests for everything and be sure that nothing breaks.

roflmaostc · 2025-08-08T12:47:39Z

You copied some code, did you check the license? Probably MIT and we need to attribute or mention it where it was copied from.

roflmaostc · 2025-08-08T12:48:57Z

+function Base.isapprox(y::AbstractArray, x::AllShiftedAndViewsCu; atol=0, rtol=atol>0 ? 0 : sqrt(eps(real(eltype(x)))),  va...)     
+    atol = (atol != 0) ? atol : rtol * maximum(abs.(x))
+    return all(abs.(x .- y) .<= atol)
+end
+


why are those methods defined again? Base should be able to handle this generically?

I am pretty sure there was a reason. The CUDA-tests did not run properly without this definition, which is why I defined it.

roflmaostc · 2025-08-08T12:49:11Z

+function Base.:(==)(x::AllShiftedAndViewsCu, y::AllShiftedAndViewsCu) 
+    return all(x .== y)
+end


isn't base able to handle this?

I am pretty sure there was a reason. The CUDA-tests did not run properly without this definition, which is why I defined it.

roflmaostc · 2025-08-08T12:49:56Z

+struct FourierJoin{T,N, AA<:AbstractArray{T, N}, D} <: AbstractArray{T, N}
    parent::AA
-    D::Int # dimension along which to apply to copy
+    # D::Int # dimension along which to apply to copy


can be deleted?

roflmaostc · 2025-08-08T12:50:04Z

+    # function FourierJoin(parent::AA, D::Int, L1::Int, do_split::Bool) where {T,N, AA<:AbstractArray{T, N}}
+    #     FourierJoin(parent, Val(D), L1, do_split)
+    # end


roflmaostc · 2025-08-08T12:50:48Z

Overall, shouldn't this go in a separate package? It seems a bit annoying to have all this weird CUDA handling in this package instead of somewhere else.

RainerHeintzmann · 2025-08-08T13:06:31Z

Overall, shouldn't this go in a separate package? It seems a bit annoying to have all this weird CUDA handling in this package instead of somewhere else.

I dont understand this extension and the changes are done to make FourierTools.jl work with CUDA, which is then does. There is very little change to the core code now and it adds a very important functionality. So what do you suggest to achieve with a different package?

RainerHeintzmann added 10 commits March 20, 2025 10:25

first changes

0874978

added Adapts stuff

0014f53

replaced test files

da99701

added CircShiftedArray.jl

a559c4f

additions to make it work in CUDA

29334fd

before streamlining

9a80dd7

first part of streamlining

89de489

almost done

ce66b60

cleanup in CUDASupportExt

48edce3

tiny bug fix in tests

3e53700

RainerHeintzmann requested a review from roflmaostc March 22, 2025 20:01

RainerHeintzmann added 6 commits March 22, 2025 21:16

removed old tests and bug-fix with unavailable CUDA hardware support.

68a7cb5

changed opt_cu to not call CuArray under non-cuda conditions

faa5677

better generation of zeros [skip ci]

2ed72a4

saved some memory on plan_conv_buffer [skip ci]

08c0295

updated NDTools dependency to 0.8

9bece7e

updated Project.toml

1713bf4

RainerHeintzmann added 5 commits April 6, 2025 11:42

back to using ShiftedArrays.jl

0ab6450

separate cuda support extension for ShiftedArray

c1ca435

removed both Extension stub

60c0cfe

better code coverage in 1d version of getindex.

5116de8

get_base_arr to identity check

5220a1f

RainerHeintzmann mentioned this pull request Apr 10, 2025

Where to host extension(s) JuliaGPU/CUDA.jl#2735

Closed

roflmaostc reviewed Jun 30, 2025

View reviewed changes

RainerHeintzmann added 2 commits June 30, 2025 14:45

bug fixes as indicated by Felix

bf0c713

added comment [skip ci]

32acf41

roflmaostc reviewed Aug 4, 2025

View reviewed changes

minor modifications

08efce8

roflmaostc reviewed Aug 8, 2025

View reviewed changes

RainerHeintzmann closed this Aug 10, 2025


		# Define the BroadcastStyle for SubArray of MutableShiftedArray with CuArray

		function Base.Broadcast.BroadcastStyle(::Type{T}) where {N, CD, T<:AllSubArrayTypeCu{N, CD}}

Uh oh!

Conversation

RainerHeintzmann commented Mar 22, 2025

Uh oh!

codecov Bot commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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!

Uh oh!

roflmaostc commented Jun 30, 2025

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!

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!

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!

roflmaostc commented Aug 8, 2025

Uh oh!

roflmaostc commented Aug 8, 2025

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!

roflmaostc commented Aug 8, 2025

Uh oh!

RainerHeintzmann commented Aug 8, 2025

Uh oh!

Reviewers

Assignees

codecov Bot commented Apr 5, 2025 •

edited

Loading