Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix rrule for rfft and ifft for CuArray #96

Closed
wants to merge 4 commits into from
Closed

Fix rrule for rfft and ifft for CuArray #96

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0e667f
fix https://github.com/JuliaMath/AbstractFFTs.jl/issues/95
ziyiyin97 Mar 28, 2023
eac7da4
scaling in place instead of type conversion
ziyiyin97 Apr 11, 2023
dc2cfab
fix immutable array by similar
ziyiyin97 Apr 12, 2023
a22c168
add more comments
ziyiyin97 Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions ext/AbstractFFTsChainRulesCoreExt.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ function ChainRulesCore.rrule(::typeof(rfft), x::AbstractArray{<:Real}, dims)
halfdim = first(dims)
d = size(x, halfdim)
n = size(y, halfdim)
scale = reshape(
[i == 1 || (i == n && 2 * (i - 1) == d) ? 1 : 2 for i in 1:n],
ntuple(i -> i == first(dims) ? n : 1, Val(ndims(x))),
)

project_x = ChainRulesCore.ProjectTo(x)
function rfft_pullback(ȳ)
x̄ = project_x(brfft(ChainRulesCore.unthunk(ȳ) ./ scale, d, dims))
dY = ChainRulesCore.unthunk(ȳ) ./ 2
selectdim(dY, halfdim, 1) .*= 2
if 2 * (n - 1) == d
selectdim(dY, halfdim, n) .*= 2
end
Copy link
Member

Choose a reason for hiding this comment

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

This seems both inefficient (many unnecessary computations) and it breaks non-mutating arrays.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Any suggestion? I used to do the code block below in the previous commit but @gaurav-arya also made a good point that this type conversion might be slow in certain cases.

scale = typeof(y)(reshape(
        [i == 1 || (i == n && 2 * (i - 1) == d) ? 1 : 2 for i in 1:n],
        ntuple(i -> i == first(dims) ? n : 1, Val(ndims(x))),
    ))

Copy link
Contributor

@gaurav-arya gaurav-arya Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to write this code to be GPU compatible, so avoiding broadcasting with CPU arrays. That's why I divide the whole array by 2, then multiply some slices of the array by 2. I wasn't sure if what I did was the right approach, so I'd appreciate any feedback on how to write it better:)

Regarding the speed issue, I benchmarked the following code before and after this PR:

using FFTW
using ChainRulesCore

function tobenchmark(x, dims)
    y, pb = rrule(rfft, x, dims)
    return pb(y)
end

julia> @btime tobenchmark(rand(1000, 1000), 1:2);
13.913 ms (71 allocations: 45.85 MiB) [BEFORE]
13.897 ms (78 allocations: 45.84 MiB) [AFTER]

Regarding the mutable array issue, that's why I used similar in the code I originally suggested, which is semantically guaranteed to return a mutable array. I agree it's not a perfect solution for the immutable array case (perhaps using Adapt.jl or ArrayInterface.jl could help with that). But also, note that this about the type of the output array rather than the input, and afaik there is no existing case in the ecosystem where the output array is immutable: FFTW converts all CPU arrays to vectors. So it's not perfect, but it did seem to fix the CUDA case which the previous approach didn't support (and with similar it would even be correct for a hypothetical static array, although admittedly not an ideal approach) -- hopefully that helps explain my reasoning :)

x̄ = project_x(brfft(dY, d, dims))
return ChainRulesCore.NoTangent(), x̄, ChainRulesCore.NoTangent()
end
return y, rfft_pullback
Expand Down Expand Up @@ -71,15 +72,15 @@ function ChainRulesCore.rrule(::typeof(irfft), x::AbstractArray, d::Int, dims)
halfdim = first(dims)
n = size(x, halfdim)
invN = AbstractFFTs.normalization(y, dims)
twoinvN = 2 * invN
scale = reshape(
[i == 1 || (i == n && 2 * (i - 1) == d) ? invN : twoinvN for i in 1:n],
ntuple(i -> i == first(dims) ? n : 1, Val(ndims(x))),
)

project_x = ChainRulesCore.ProjectTo(x)
function irfft_pullback(ȳ)
x̄ = project_x(scale .* rfft(real.(ChainRulesCore.unthunk(ȳ)), dims))
dX = rfft(real.(ChainRulesCore.unthunk(ȳ)), dims) .* invN .* 2
selectdim(dX, halfdim, 1) ./= 2
if 2 * (n - 1) == d
selectdim(dX, halfdim, n) ./= 2
end
x̄ = project_x(dX)
return ChainRulesCore.NoTangent(), x̄, ChainRulesCore.NoTangent(), ChainRulesCore.NoTangent()
end
return y, irfft_pullback
Expand Down Expand Up @@ -111,14 +112,15 @@ function ChainRulesCore.rrule(::typeof(brfft), x::AbstractArray, d::Int, dims)
# compute scaling factors
halfdim = first(dims)
n = size(x, halfdim)
scale = reshape(
[i == 1 || (i == n && 2 * (i - 1) == d) ? 1 : 2 for i in 1:n],
ntuple(i -> i == first(dims) ? n : 1, Val(ndims(x))),
)

project_x = ChainRulesCore.ProjectTo(x)
function brfft_pullback(ȳ)
x̄ = project_x(scale .* rfft(real.(ChainRulesCore.unthunk(ȳ)), dims))
dX = rfft(real.(ChainRulesCore.unthunk(ȳ)), dims) .* 2
selectdim(dX, halfdim, 1) ./= 2
if 2 * (n - 1) == d
selectdim(dX, halfdim, n) ./= 2
end
x̄ = project_x(dX)
return ChainRulesCore.NoTangent(), x̄, ChainRulesCore.NoTangent(), ChainRulesCore.NoTangent()
end
return y, brfft_pullback
Expand Down