wand bins patch: fix #479

[mkborregaard]

Seemed generally bitrotted. Missing a dep for the FFT, the pdf function was changed in Distributions long ago. For wand bins to actually work also requires JuliaPlots/Plots.jl#4076 in Plots (version XX)

[sethaxen]

I'd prefer we just used the AbstractFFTs interface. 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. So we can document that to use wand bins, the user should load an FFT package (e.g. FFTW or FastTransforms). And if one of the other packages in their environment already did, then great, it already works.

[mkborregaard]

Are you sure? That sounds strange, why would the user not be able to choose a different FFT than the one we use internally here? Not really keen on hoisting the dep management manually onto the user.

[sethaxen]

Are you sure? That sounds strange, why would the user not be able to choose a different FFT than the one we use internally here?

Currently only if the user constructs their own FFT plan can they coerce the backend. The fft function takes no backend argument, so it ends up using one of the packages that has been loaded but the user can't control which. See JuliaMath/AbstractFFTs.jl#32

Not really keen on hoisting the dep management manually onto the user.

I think this is standard for FFT though. I'll look into it some more.

[mkborregaard]

So the FFTW dep is a bit strange to me - this functionality obviously worked when it was added, so fft must have been in the namespace without FFTW at that time. What package defined that?
@piever do you happen to know this?

[piever]

We depend on KernelDensity for the smooth density plots, and they load FFTW (see here), so even if we get the function from AbstractFFT (which I guess we might), there will already be the FFTW backend loaded due to KernelDensity.

I personally would also prefer it if KernelDensity was FFTW backend agnostic (due to the MKL dependency of FFTW.jl), but

1. that should be probably discussed over at KernelDensity.jl, and
2. it would IMO only really make sense once there is more than one implementation of FFT for cpu arrays.

[sethaxen]

So the FFTW dep is a bit strange to me - this functionality obviously worked when it was added, so fft must have been in the namespace without FFTW at that time. What package defined that?

According to the AbstractFFTs docs, it used to be in the stdlib for Julia 0.6 and earlier. StatsPlots dropped support for v0.6 in v0.8.0. Wand bins were added in v0.5.0.

[sethaxen]

We depend on KernelDensity for the smooth density plots, and they load FFTW (see here), so even if we get the function from AbstractFFT (which I guess we might), there will already be the FFTW backend loaded due to KernelDensity.

Ah okay, well that then makes this discussion moot. But I would prefer to just depend on AbstractFFTs directly then.

[mkborregaard]

@sethaxen I understand your concern as it seems we are adding a new binary dep, but actually KernelDensity here already depends directly on FFTW, so there is no difference in terms of footprint. If you prefer, I can do import KernelDensity.FFTW: fft? Though I think it is better practice to make the dep explicit.

EDIT: Ah sorry, just saw now that @piever noted the same thing above before I typed

[mkborregaard]

@sethaxen alright, I've updated the dep to AbstractFFTs. Seems to work perfectly. I also added a test that isn't numerically precise (because one shouldn't assume random numbers in tests) but just sees if the function runs essentially.
For some reason the Distribution tests fail on my machine when I run the StatsPlots tests locally, but that is orthogonal.

[sethaxen]

A few minor changes

[mkborregaard]

Changed as requested.

[sethaxen]

LGTM!