search

JuliaDiff / ChainRules.jl

forward and reverse mode automatic differentiation primitives for Julia Base + StdLibs
Other
434 stars 89 forks source link

Rules for FFT #127

Open jessebett opened 5 years ago

jessebett commented 5 years ago

It appears there are adjoint rules for FFT in Zygote here: https://github.com/FluxML/Zygote.jl/pull/215

jessebett commented 5 years ago

Here is the relevant file in Zygote src: https://github.com/FluxML/Zygote.jl/blob/a1e3f9ffbf49fd710ab2f117e3ee4138a6677aa7/src/lib/array.jl#L539-L585

roflmaostc commented 3 years ago

Hey, today I was missing a rule for fft!. The easiest way is to add that missing lines directly to Zygote, however in the long run it would be better to invest some work so that it's based on ChainRules.

Should the rules sit here or in AbstractFFTs?

@oxinabox What do you think about that?

devmotion commented 3 years ago

Apparently @stevengj suggested they should be moved to AbstractFFTs: https://github.com/JuliaMath/AbstractFFTs.jl/issues/47#issuecomment-741914478 and https://github.com/FluxML/Zygote.jl/issues/835#issuecomment-741902634

roflmaostc commented 3 years ago

Thanks a lot for the useful links. I'll check it out whether I can come up with a PR fixing that issue

mcabbott commented 3 years ago

Note that AbstractFFTs seems to be a much smaller, and much slower-changing, package than ChainRulesCore. Especially if you include a test dep on ChainRulesTestUtilititesEtc, which is also changing. So I'd still vote that they should move here. 

julia> @time using ChainRulesCore
  0.210714 seconds (621.77 k allocations: 37.128 MiB, 1.89% gc time)

julia> @time using AbstractFFTs
  0.008525 seconds (18.83 k allocations: 1.371 MiB)

julia> 0.21 / 0.0085
24.705882352941174
oxinabox commented 3 years ago

Note that AbstractFFTs seems to be a much smaller, and much slower-changing, package than ChainRulesCore. Especially if you include a test dep on ChainRulesTestUtilititesEtc, which is also changing. So I'd still vote that they should move here.

I think they should just wait for ChainRulesCore 1.0 to be tagged, which is not long away and then they can be in AbstractFFTs. ChainRulesCore 1.0 will be much lighter (we are doing something that julia apparently is really bad at precompiling, we will stop doing that), and also will be stable. Aim is to have ChainRulesCore 1.0 done by JuliaCon

roflmaostc commented 3 years ago

Well, technically I can work on that PR because if people decide to move it elsewhere we already would have the code then. (Basically it's a translation from Zygote.@adjoint to rrule)

devmotion commented 3 years ago

I made a PR for AbstractFFTs: https://github.com/JuliaMath/AbstractFFTs.jl/pull/58