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.

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

Rules for FFT #127

Open
jessebett opened this issue Oct 22, 2019 · 8 comments
Open

Rules for FFT #127

jessebett opened this issue Oct 22, 2019 · 8 comments

Comments

@jessebett
Copy link

It appears there are adjoint rules for FFT in Zygote here:
FluxML/Zygote.jl#215

@jessebett
Copy link
Author

@roflmaostc
Copy link

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
Copy link
Member

Apparently @stevengj suggested they should be moved to AbstractFFTs: JuliaMath/AbstractFFTs.jl#47 (comment) and FluxML/Zygote.jl#835 (comment)

@roflmaostc
Copy link

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

@mcabbott
Copy link
Member

mcabbott commented May 7, 2021

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
Copy link
Member

oxinabox commented May 7, 2021

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
Copy link

roflmaostc commented May 7, 2021

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
Copy link
Member

I made a PR for AbstractFFTs: JuliaMath/AbstractFFTs.jl#58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants