JuliaLinearAlgebra / AppleAccelerate.jl

Julia interface to the macOS Accelerate framework
Other
96 stars 18 forks source link

Updated DCT implementation with documentation #16

Closed rprechelt closed 8 years ago

rprechelt commented 8 years ago

I updated the existing DCT implementation that someone contributed to be consistent with the rest of the project. I also added new docstrings to list parameter restrictions as defined by Accelerate.

tkelman commented 8 years ago

I think the plan terminology is used in Base with FFTW

rprechelt commented 8 years ago

I was looking closely at FFTW.jl while writing this, and yes, plan_dct is used by Base.

I guess it comes down to whether this package is meant to be a drop-in replacement for Base functions or a true Julia interface to Accelerate that follows the calling conventions of Accelerate? The Accelerate version of dct supports extra forms/types then the one in Base and this should be to be exposed (but this could be done with optional arguments while still preserving plan_dct syntax)..

I'm totally happy to rename this to be consistent with Base, but some clarification on the package standard for this issue might be helpful. @simonbyrne any thoughts? There are a lot of small differences between Base and Accelerate with respect to the FFT, DFT and other functions, and having a standard to resolve this would be great.

simonbyrne commented 8 years ago

I'm not very familiar with FFTW, or the accelerate DSP functions, but if they are mostly used in a similar manner (even if there are some different optional arguments), then I think it would makes sense to use the same names.

rprechelt commented 8 years ago

Ok! I'll make the change momentarily and update the PR.

rprechelt commented 8 years ago

I updated the names to be consistent with Base.dct. I also added a destructor for DCT plan's that was not originally part of the commit. This has been incorporated into the testing.

rprechelt commented 8 years ago

Changed DFT_Setup to DFTSetup to be more consistent with Julia conventions.

rprechelt commented 8 years ago

Changed.

simonbyrne commented 8 years ago

Looks good to me. You'll need to rebase it though.