Combined FP/DP & Guru interface v2

[jkrimmer]

By combining the work of Libin (see guru_v2 branch and I, we end up with this new PR for the combined FP/DP & Guru interface. Thanks to @lu1and10, there are even unit tests for the guru interface in one dimension included. Since the opts struct is now mostly hidden from the user as keyword arguments enable the configuration of finufft, we probably need a major version bump. In doing so, we could also safely avoid the explicit FP interfaces.

[codecov[bot]]

Codecov Report

Merging #31 (01d7dec) into master (4fdb755) will decrease coverage by 2.21%. The diff coverage is 86.44%.

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   88.02%   85.80%   -2.22%     
==========================================
  Files           2        3       +1     
  Lines         192      303     +111     
==========================================
+ Hits          169      260      +91     
- Misses         23       43      +20     

Impacted Files	Coverage Δ
src/FINUFFT.jl	60.00% <59.64%> (-22.58%)	:arrow_down:
src/guru.jl	80.89% <80.89%> (ø)
src/simple.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4fdb755...01d7dec. Read the comment docs.

[lu1and10]

By combining the work of Libin (see guru_v2 branch and I, we end up with this new PR for the combined FP/DP & Guru interface. Thanks to @lu1and10, there are even unit tests for the guru interface in one dimension included. Since the opts struct is now mostly hidden from the user as keyword arguments enable the configuration of finufft, we probably need a major version bump. In doing so, we could also safely avoid the explicit FP interfaces.

Hi Jonas,

Thanks for the merge and new PR. One major change you made is to split the guru functions into two overloading functions(SP and DP). finufft_setpts and finufft_exec depends on the input data precision T, this may introduce code replications as for each guru function we need two copy, one for SP and one for DP.

For finufft_makeplan, it depends only on eps::T to decide precision type which seems to be not standard, that's why @ahbarnett and I added the dtype keyword for user to specify precision types explicitly.

@ludvigak Do you have any opinion? Should we maintain a dtype keywords(https://github.com/lu1and10/FINUFFT.jl/blob/guru_v2/src/FINUFFT.jl) or we use Jonas overloading functions(the downside is we may have some duplications)

Best, Libin

[jkrimmer]

Hi Libin,

thanks a lot for the review! This introduction of duplicates is intended to make use of julia's concept of multiple dispatch. However, I am not fully satisfied with my approach of controlling the transform precision with the type of eps only. I'm looking forward to your suggestions :)

Best, Jonas

[ahbarnett]

Dear Libin, Jonas, Ludvig,

I agree julia's multiple-dispatch was a good idea for native julia code, but I think having an explicit dtype arg is better, to match Py and Matlab, since really this is just a wrapper to C++ code. Avoiding code duplication is important, since it makes future changes easier. I am happy to test out the PR once ready. Thank-you all, Best, Alex

On Tue, Jun 8, 2021 at 3:51 PM Jonas Krimmer @.***> wrote:

Hi Libin,

thanks a lot for the review! This introduction of duplicates is intended to make use of julia's concept of multiple dispatch. However, I am not fully satisfied with my approach of controlling the transform precision with the type of eps only. I'm looking forward to your suggestions :)

Best, Jonas

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ludvigak/FINUFFT.jl/pull/31#issuecomment-857066377, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNZRSUBYWFHR7XV37I6M7DTRZYD5ANCNFSM46DFBSYA .

-- *---------------------------------------------------------------------~^`^~._.~' |\ Alex H. Barnett Center for Computational Mathematics, Flatiron Institute | \ http://users.flatironinstitute.org/~ahb 646-876-5942

[ludvigak]

Hi all, My initial idea for this interface was to make a transparent interface to the C++ library, that would mimic all its function calls (and their arguments). That way, one can read the FINUFFT docs and use this interface. I still think this is a good principle (and also one that doesn't break our interface).

Then we can wrap usage of Julia's multiple dispatch on top of that to make the interface seamless to the user, but that is just icing on the cake in my mind, and I only think it really matters for the non-guru interfaces. If a user believes they have what it takes to be a guru, then I think we can expect them to use finufftf instead of finufft when they need to ;)

[jkrimmer]

Dear all,

thanks a lot for your feedback. Fortunately, we can get rid of the code duplication by using metaprogramming and generating the matching functions in a loop. Then, the makeplan function would look something like this:

for (T,cfun) ∈ ((Float64,(:finufft_makeplan,libfinufft)), (Float32,(:finufftf_makeplan,libfinufft)))
    @eval begin
        function finufft_makeplan(type::Integer,
                            n_modes_or_dim::Union{Array{BIGINT},Integer},
                            iflag::Integer,
                            ntrans::Integer,
                            eps::$T;
                            kwargs...)
        # see https://stackoverflow.com/questions/40140699/the-proper-way-to-declare-c-void-pointers-in-julia for how to declare c-void pointers in julia
        #   one can also use Array/Vector for cvoid pointer, Array and Ref both work
        #   plan_p = Array{finufft_plan_c}(undef,1)
            plan_p = Ref{finufft_plan_c}()

            opts = finufft_default_opts(nufft_opts{$T}())
            setkwopts!(opts;kwargs...)

            n_modes = ones(BIGINT,3)
            if type==3
                @assert ndims(n_modes_or_dim) == 0
                dim = n_modes_or_dim
            else
                @assert length(n_modes_or_dim)<=3 && length(n_modes_or_dim)>=1
                dim = length(n_modes_or_dim)
                n_modes[1:dim] .= n_modes_or_dim
            end

            ret = ccall( $cfun,
                            Cint,
                            (Cint,
                            Cint,
                            Ref{BIGINT},
                            Cint,
                            Cint,
                            $T,
                            Ptr{finufft_plan_c},
                            Ref{nufft_opts}),
                            type,dim,n_modes,iflag,ntrans,eps,plan_p,opts
                            )

            check_ret(ret)

            ms = n_modes[1]
            mt = n_modes[2]
            mu = n_modes[3]
            plan = finufft_plan{$T}(type,ntrans,dim,ms,mt,mu,0,0,plan_p[])
        end
    end
end

This way, even the proposal by Ludvig of having finufftf calls coexist with finufft ones would not require an additional line of code (adding a third function name parameter to the for-loop would suffice). IMHO, the only downside to this approach is the lack of a reasonable dtype parameter. However, since this procedure affects only the guru functions, we could still take the dtype approach for the basic interface and have something like

function nufft1d1(xj::Array{T},
                  cj::Array{Complex{T}},
                  iflag::Integer,
                  eps::T,
                  ms::Integer;
                  dtype::DataType=T,
                  kwargs...) where T <: fftwReal
    tol = dtype(eps)
    valid_setpts(1,1,xj)
    ntrans = valid_ntr(xj,cj)
    fk = Array{Complex{dtype}}(undef, ms, ntrans)
    checkkwdtype(dtype; kwargs...)
    nufft1d1!(xj, cj, iflag, tol, fk; kwargs...)
    return fk
end

where dtype can be used to override the data type. What do you think?

Best Jonas

[jkrimmer]

Dear all, I have finally updated this pull request to incorporate my proposed changes (see comment from June 28). Best Jonas

[ahbarnett]

Dear Jonas, Libin & I have now brought in your changes, and edited as we see fit, via a different PR. Hence I'm closing this obsolete one. Your code has been very useful - thank-you. A couple of changes from your version are:

• explicit dtype arg controls Float32/Float64 transform precision (also in tests)
• no metaprogramming for now (but thanks for the idea)
• tol (eps) can be any Real.
• certain other things like renaming fftwReal to finufftReal.

We hope you agree it's a good compromise in terms of matching the other language interfaces. Best wishes, Alex

[jkrimmer]

Dear Alex,

thanks to Libin and you for putting everything together! I am already looking forward to the new release.

Best wishes Jonas

[ahbarnett]

Hi Jonas, it's ready now if you want to check it. Also see To Do in README... Cheers, Alex

On Tue, Nov 2, 2021 at 7:04 AM Jonas Krimmer @.***> wrote:

Dear Alex,

thanks to Libin and you for putting everything together! I am already looking forward to the new release.

Best wishes Jonas

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ludvigak/FINUFFT.jl/pull/31#issuecomment-957336008, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNZRSTDWHRF4GQZDV5RFWLUJ7ASLANCNFSM46DFBSYA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- *---------------------------------------------------------------------~^`^~._.~' |\ Alex H. Barnett Center for Computational Mathematics, Flatiron Institute | \ http://users.flatironinstitute.org/~ahb 646-876-5942