FluxML / Zygote.jl

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

Move type-pirating methods to `FillArrays`? #1426

Open jishnub opened 1 year ago

jishnub commented 1 year ago

E.g. https://github.com/FluxML/Zygote.jl/blob/17ca911b82134c4a765822cd2b7ee19e959cc8e4/src/lib/array.jl#L783

This looks like it should belong to an extension in FillArrays.jl

devmotion commented 1 year ago

Seems one could exploit though that

$$ \begin{split} DFT(A)[k] &= \sum^{length(A)}_{n=1} \exp{\bigg(\frac{- 2\pi i (k - 1) (n - 1)}{length(A)}\bigg)} A[n] \ &= \begin{dcases} N A[1] & \text{if } k = 1,\ \frac{1 - \exp{(-2 \pi i (k- 1))}}{1 - \exp{\Big(\frac{-2 \pi i (k - 1)}{length(A)}\Big)}} A[1] & \text{if } k > 1, \end{dcases} \ &= \begin{dcases} N A[1] & \text{if } k = 1,\ 0 & \text{if } k > 1, \end{dcases} \end{split} $$

if all entries of $A$ are equal.

Edit: Simplified even more (see below).

jishnub commented 1 year ago

Yes, indeed, but won't the second case be zero for integer 1 < k <= length(A)? A constant array should only have the 0-frequency component populated.

devmotion commented 1 year ago

Yep, I just noticed that I could have simplified it even more 😅

ToucheSir commented 1 year ago

You somehow discovered a very old version of that file. The current Zygote codebase doesn't use FFTW at all: https://github.com/FluxML/Zygote.jl/blob/master/src/lib/array.jl

devmotion commented 1 year ago

It's still there, just using AbstractFFT instead of FFTW (which I assume one would also want to use in an extension rather than FFTW): https://github.com/FluxML/Zygote.jl/blob/e2d7839a476fc1cda407500d9a2ca125bd6e2aa5/src/lib/array.jl#L672-L677

ToucheSir commented 1 year ago

Right, but for that we already have https://github.com/FluxML/Zygote.jl/issues/835 no? AbstractFFTs shouldn't really be a dep of Zygote in the first place.

devmotion commented 1 year ago

IMO it's related to #835 but not a duplicate (I guess one could view it differently though): #835 is about reverse mode rules whereas this issue is just about blatant type piracy without involving any rules. It just happens that the same package (AbstractFFTs) is involved in both.

ToucheSir commented 1 year ago

Yes, my point is that removing the rules for AbstractFFTs from Zygote should also involve removing any piracy (because there should be no code referring to AbstractFFTs left in Zygote afterwards).

dlfivefifty commented 1 year ago

Yes make it an extension . But shouldn’t it return a OneHotVector ?

devmotion commented 1 year ago

That was our point above :slightly_smiling_face: