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

FFTW overrides via extension #668

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dlfivefifty
Copy link
Contributor

This replaces #541 and introduces overrides needed for FFTW as extensions.

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

@dlfivefifty
Copy link
Contributor Author

@dlfivefifty
Copy link
Contributor Author

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

@devmotion
Copy link
Member

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

@dlfivefifty
Copy link
Contributor Author

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

@devmotion
Copy link
Member

It's not an extension on Julia < 1.9.

@dlfivefifty
Copy link
Contributor Author

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

@codecov
Copy link

codecov bot commented Oct 2, 2023

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devmotion
Copy link
Member

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.

@antoine-levitt
Copy link
Contributor

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?...)

@devmotion
Copy link
Member

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).

@antoine-levitt
Copy link
Contributor

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?

@dlfivefifty
Copy link
Contributor Author

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

@devmotion
Copy link
Member

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. JuliaMath/AbstractFFTs.jl#32, JuliaStats/KernelDensity.jl#117, and the discussion in JuliaPlots/StatsPlots.jl#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.

@sjkelly
Copy link
Contributor

sjkelly commented Nov 30, 2023

Is using Requires on <1.9 an option here?

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

Successfully merging this pull request may close these issues.

4 participants