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

Use AbstractFFTs instead of FFTW #117

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

Use AbstractFFTs instead of FFTW #117

wants to merge 3 commits into from

Conversation

devmotion
Copy link
Member

The AbstractFFTs interface is a bit special: The only way to select a backend such as FFTW or RustFFT is to load the desired backend package (see e.g. JuliaMath/AbstractFFTs.jl#32 and the discussion in JuliaPlots/StatsPlots.jl#480). In particular, as noted in JuliaPlots/StatsPlots.jl#480 (comment) this means that

Once one of the packages that implements the interface is loaded, there's no way for the user to switch between them, and the whole point of the interface package is to not coerce the user to a choice.

Generally, that seems to be a questionable design decision to me. However, until the design is changed, IMO KernelDensity.jl should follow it and not enforce the use of FFTW.jl throughout the ecosystem, in particular since nowadays different backends such as FFTW.jl, GenericFFTs.jl, and RustFFT.jl (real FFTs planned but not supported yet) exist. Even more so since the GPL-license of FFTW can be problematic and MKL alternative is a large lazy artifact and missing support for e.g. recent Macs with Apple silicon.

@devmotion
Copy link
Member Author

AFAICT the test errors on Julia 1.0 are caused by the way that Pkg resolves and installs test dependencies on such old Julia versions. When the package is built, the latest compatible versions of the (non-test) dependencies are installed - but no version of FFTW is compatible with those AND supports Julia 1.0. On more recent Julia versions Pkg would resolve and reinstall (non-test + test) dependencies in such a case, but on Julia 1.0 Pkg does not do that and hence tests fail.

I wonder, however, if it is actually required to support Julia 1.0. Dropping support for anything older than Julia 1.6, the latest LTS version, might be fine?

@tpapp
Copy link
Collaborator

tpapp commented Oct 3, 2023

FWIW, I would just wait for a better interface to emerge. Is there any fundamental issue preventing this?

@devmotion
Copy link
Member Author

I don't expect the AbstractFFTs design to change any time soon since nothing has happened in the last four years (Keno opened the issue in AbstractFFTs linked above in August 2019). The problem is that KernelDensity is used by many packages, indirectly and directly, and given the number of alternative backends and the problems with FFTW.jl listed above the current design of KernelDensity does not play nicely with other packages in the ecosystem.

@ChrisRackauckas
Copy link

This clearly should be done. I don't see why a package like this would choose a backend anyways, regardless of the oddities around switching.

@tpapp
Copy link
Collaborator

tpapp commented Dec 10, 2023

Is there a way we can provide a specific error message in case the user does not load any FFTW backend libraries? Eg suggest that they load one, maybe FFTW. I am OK with breaking changes, but a nice suggestion would help.

@devmotion
Copy link
Member Author

devmotion commented Dec 10, 2023

I don't think there is any other way to detect whether a AbstractFFTs backend is available than calling the FFT methods and checking whether a MethodError is thrown. To improve the error messages I guess AbstractFFTs could add error hints to the FFT methods that backends should overload (https://docs.julialang.org/en/v1/base/base/#Base.Experimental.register_error_hint).

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.

3 participants