search

FluxML / Zygote.jl

21st century AD
https://fluxml.ai/Zygote.jl/
Other
1.48k stars 213 forks source link

Zygote depends on FFTW? #553

Closed Byrth closed 4 years ago

Byrth commented 4 years ago

Today I discovered that Zygote.jl lists FFTW.jl as a dependency. FFTW.jl, in turn, depends on FFTW_jll and MKL_jll (the two available backends).

The argument made on the FFTW.jl github page indicates that they believe users are not bound by the GPL license as long as they are using the MKL backend, but the default is FFTW and thus most users of the package and anything that depends on it are getting default upgraded to GPL.

Two questions, then:

  1. Does Zygote actually depend on FFTW, or is it merely a convenient method extension? ( https://github.com/FluxML/Zygote.jl/issues/204 )
  2. If it is the latter, would you consider switching to using Requires for it?

If the answer is "convenient method extension" and "no," do you foresee any problems arising if I scrape it out of my fork? I'm only using my fork with Flux right now.

mcabbott commented 4 years ago

I thought this is the problem https://github.com/JuliaMath/AbstractFFTs.jl was invented to solve. Couldn't the gradients defined here use that instead?

https://github.com/FluxML/Zygote.jl/blob/f16d85637a651d19371d88ae96e3f82a666202d4/src/lib/array.jl#L789

I guess there are also tests...

Byrth commented 4 years ago

Extending AbstractFFTs and switching FFTW to be a test dependency would fix this issue, I believe.

MikeInnes commented 4 years ago

I agree it'd be good to get rid of the binary/GPL dependencies (didn't know about MKL, that's even worse). If someone can switch us over to AbstractFFTs in a PR that'd certainly be appreciated.

Byrth commented 4 years ago

I would volunteer to produce the PR, but I haven't used the FFT interfaces for Julia and don't want to break something by not understanding their nuances.

dsweber2 commented 4 years ago

I wrote a implementation using AbstractFFTs a while ago when I needed the adjoints of fft plans as well. As far as I can tell it just needs to be merged, though perhaps the tests could be done more cleanly.

Byrth commented 4 years ago

Merged and released in 0.4.13