ForwardDiff.jl icon indicating copy to clipboard operation
ForwardDiff.jl copied to clipboard

FFTW overrides via extension

Open dlfivefifty opened this issue 2 years ago • 14 comments

This replaces https://github.com/JuliaDiff/ForwardDiff.jl/pull/541 and introduces overrides needed for FFTW as extensions.

Note it will make FFTW and AbstractFFTs dependencies in Julia <v1.9

dlfivefifty avatar Oct 02 '23 19:10 dlfivefifty

This also replaces https://github.com/JuliaApproximation/FastTransformsForwardDiff.jl

dlfivefifty avatar Oct 02 '23 19:10 dlfivefifty

I don't understand whats causing the Invalidations failure, it looks like an unrelated bug...

dlfivefifty avatar Oct 02 '23 20:10 dlfivefifty

IMO ForwardDiff should not depend on FFTW. Instead I think it would be preferable to fix https://github.com/JuliaMath/AbstractFFTs.jl/issues/56 and only depend on AbstractFFTs.

devmotion avatar Oct 02 '23 20:10 devmotion

This is why its an extension so it doesn't depend on either but I agree with you that https://github.com/JuliaMath/AbstractFFTs.jl/issues/56 makes sense

dlfivefifty avatar Oct 02 '23 20:10 dlfivefifty

It's not an extension on Julia < 1.9.

devmotion avatar Oct 02 '23 20:10 devmotion

I believe we can just drop FFTW support on Julia <v1.9. How does that sound?

dlfivefifty avatar Oct 02 '23 20:10 dlfivefifty

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d300209) 89.65% compared to head (1130fa4) 86.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   89.65%   86.63%   -3.03%     
==========================================
  Files          11       13       +2     
  Lines         967      920      -47     
==========================================
- Hits          867      797      -70     
- Misses        100      123      +23     
Files Coverage Δ
ext/ForwardDiffFFTWExt.jl 100.00% <100.00%> (ø)
ext/ForwardDiffAbstractFFTsExt.jl 90.90% <90.90%> (ø)
src/complex.jl 60.00% <60.00%> (ø)

... and 10 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 02 '23 20:10 codecov[bot]

I guess this would be annoying in practice. Downstream packages that want to use ForwardDiff + FFTW would have to copy the diff rules or add some workaround for them in Julia < 1.9, including the LTS version.

devmotion avatar Oct 03 '23 08:10 devmotion

Would something like load_FFTW_rules() = include(@__DIR__ * "/fftw_rules.jl") work, so downstream packages can opt in manually for julia < 1.9? (but honestly, do people really use old julia versions?...)

antoine-levitt avatar Oct 03 '23 08:10 antoine-levitt

One could also put the FFTW rules behind a Preference, similar to the NaN-safe mode. But this would only prevent ForwardDiff from loading the file by default, FFTW would still have to be a proper dependency on Julia < 1.9. Generally the GPL-license of FFTW and the size of the MKL alternative (which e.g. doesn't work on ARM and hence can't be used in more recent Macs) make FFTW a quite unattractive dependency in various applications (e.g., in Pumas we would like to get rid of FFTW.jl rather sooner than later but unfortunately a few packages in the ecosystem still depend on FFTW.jl and would have to be switched to AbstractFFTs.jl or some other FFT backend first).

devmotion avatar Oct 03 '23 08:10 devmotion

Looking at the patch, the bulk of it only requires AbstractFFTs, FFTW is only needed for real-to-real FFTs, so just drop FFTW for <1.9 and <1.9 users can still enjoy most of the benefits of this, and add the relatively small real-to-real FFTs by hand if they really need it?

antoine-levitt avatar Oct 03 '23 08:10 antoine-levitt

I’m not quite sure what to do about the tests failures as they pass locally and it’s unclear what’s causing allocations

dlfivefifty avatar Oct 03 '23 09:10 dlfivefifty

Not supporting the FFTW-specific rules on Julia < 1.9 might be a bit surprising but I think it would still be much more preferable than depending on FFTW. The current design of AbstractFFTs is to leave the choice of the FFT backend to the users - as soon as a package loads an FFT backend (directly or indirectly) it's used for all FFT computations in all packages, i.e., the AbstractFFTs design assumes that only users load a backend because otherwise the whole ecosystem is affected (see, e.g., see e.g. https://github.com/JuliaMath/AbstractFFTs.jl/issues/32, https://github.com/JuliaStats/KernelDensity.jl/pull/117, and the discussion in https://github.com/JuliaPlots/StatsPlots.jl/pull/480). I think this design is questionable but probably its restrictions were not a practical concern as long as there was no proper alternative to FFTW.jl. However, with alternatives such as RustFFT.jl nowadays it's more problematic if packages do not take the AbstractFFTs design into account.

devmotion avatar Oct 04 '23 09:10 devmotion

Is using Requires on <1.9 an option here?

sjkelly avatar Nov 30 '23 20:11 sjkelly