search

JuliaStats / KernelDensity.jl

Kernel density estimators for Julia
Other
178 stars 40 forks source link

Use AbstractFFTs instead of FFTW #117

Open devmotion opened 1 year ago

devmotion commented 1 year ago

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. https://github.com/JuliaMath/AbstractFFTs.jl/issues/32 and the discussion in https://github.com/JuliaPlots/StatsPlots.jl/pull/480). In particular, as noted in https://github.com/JuliaPlots/StatsPlots.jl/pull/480#issuecomment-1022370329 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 commented 1 year ago

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 commented 1 year ago

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

devmotion commented 1 year ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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