Use a simple DFT algorithm when FFTW is not in Base #641
Conversation
|
The nightly failures are due to unescaped backslashes in docstrings, which will hopefully be fixed by #640. |
| if VERSION >= v"0.7.0-DEV.602" | ||
| function _dft(x::Vector{T}) where T | ||
| n = length(x) | ||
| y = fill!(copy(x), zero(complex(float(T)))) |
There was a problem hiding this comment.
Please look at this one again. I'm pretty sure it is not doing what you intended it to do. Furthermore, there is no reason to initialize the memory since that is done in the loop below.
| n = length(x) | ||
| y = fill!(copy(x), zero(complex(float(T)))) | ||
| @inbounds for j = 0:n-1, i = 0:n-1 | ||
| y[k+1] += x[j+1] * cis(-2π * j * k / n) |
There was a problem hiding this comment.
k should be i or vice versa.
Calling this Also the things that Andreas noted are really embarrassing. 😐 Sorry about that... |
|
Btw. I looked at the |
|
@ararslan Ah, I see. My mistake. I got confused about it being a drop-in for |
andreasnoack
changed the title
|
Looks like this isn't retaining sufficient accuracy for the Poisson binomial PDF tests to pass. In particular: We could widen the tolerance for the test or find a better algorithm... |
|
If the tolerance is widened, would it be helpful to issue a warning (at least until this workaround is unnecesary)? |
|
The workaround is more or less permanent; the FFTW bindings (and the FFT functionality as a whole) were removed from Base in Julia 0.7 and this replaces the uses of it with a simple FFT implementation. |
|
I see. I had it the wrong way around.
In the failed test, your `_dft` must be losing precision somehow, right?
Does the test pass robustly for `fft` with different values/seeds, or is
this one of those tests that fails some percentage of the time if we're not
careful?
…On Sun, Jul 16, 2017 at 21:52 Alex Arslan ***@***.***> wrote:
The workaround is more or less permanent; the FFTW bindings (and the FFT
functionality as a whole) were removed from Base in Julia 0.7 and this
replaces the uses of it with a simple FFT implementation.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#641 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADBM50qDbvSDt0D2Oax-ebPGGBMjQMzrks5sOr5rgaJpZM4OZJ22>
.
|
|
I believe that tests here are deterministic. It loops through a fixed set of parameter values and compares the |
|
Then maybe lower test tolerance as suggested and make a note about limited
tail accuracy in the docs?
…On Sun, Jul 16, 2017 at 22:42 Andreas Noack ***@***.***> wrote:
I believe that tests here are deterministic. It loops through a fixed set
of parameter values and compares the pdf with a slower more precise
version. I printed all the values for the previous FFTW based version and
it produced values around 0.0 and eps() while the new one is up to 8eps()
so neither of them have any relative accuracy in the tail. If we really
care about the relative accuracy, we should probably change algorithm
completely to the version used in the tests but I'm not sure accuracy at
that level is required.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#641 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADBM5xZjYyfIYPVjiXJUSIyvIVXHlNHOks5sOsoKgaJpZM4OZJ22>
.
|
|
Good idea. Let's merge #640 first so that the note in the docs can be applied in the right place. |
Avoids having to introduce a dependency on FFTW, which has binary dependencies, just for this one distribution. Distributions is already a pretty heavyweight dependency for packages; FFTW (once it actually works) will only make that worse. Thus I'm proposing this simple, standalone DFT implementation for the Poisson binomial PDF.