Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

finufft/cufinufft interface rationalization (C) #255

Closed
janden opened this issue Mar 17, 2023 · 6 comments · Fixed by #337
Closed

finufft/cufinufft interface rationalization (C) #255

janden opened this issue Mar 17, 2023 · 6 comments · Fixed by #337

Comments

@janden
Copy link
Collaborator

janden commented Mar 17, 2023

Currently, the C interfaces for finufft and cufinufft are quite close, but don't match up exactly, which could lead to confusion. We currently have:

Makeplan

int finufft_makeplan(int type, int dim, int64_t* nmodes, int iflag, int ntr, double eps, finufft_plan* plan, nufft_opts* opts)
int cufinufft_makeplan(int type, int dim, int* nmodes, int iflag, int ntransf, double tol, int maxbatchsize, cufinufft_plan *plan, cufinufft_opts *opts)

The proposed solution here is to remove maxbatchsize (i.e., move it into opts), use int64_t for nmodes and rename ntransf and tol.

Setpts

int finufft_setpts(finufft_plan plan, int64_t m, double* x, double* y, double* z, int64_t n, double* s, double* t, double* z)
int cufinufft_setpts(int m, double* x, double* y, double* z, int n, double* s, double* t, double *u, cufinufft_plan plan)

Proposed solution is to move plan to first argument and use int64_t.

Execute

int finufft_execute(finufft_plan plan, complex double* c, complex double* f)
int cufinufft_execute(cuDoubleComplex* c, cuDoubleComplex* f, cufinufft_plan plan)

Again, plan should be moved to the beginning.

Destroy
Same.

Default_opts

void finufft_default_opts(finufft_opts* opts)
int cufinufft_default_opts(int type, int dim, cufinufft_opts* opts)

There are a few things to deal with here. We'd like to be able to call default_opts in cufinufft without specifying dimension or type (this should also allow us to return void as well). To achieve this, certain options need to have an “auto” setting, where their exact values are set during makeplan depending on dim and opts. Looking at cufinufft_default_opts, it looks like the only option that is dependent on the type (there is no dependence on dimension) is gpu_method (method #2 for type 1 and method #1 for type 2), so this should be fairly straightforward (specify gpu_method = 0 for “auto”).

@janden
Copy link
Collaborator Author

janden commented Dec 26, 2023

So while writing up the docs, I'm realizing that we forgot one change: in cufinufft_setpts, we still have int for the number of points, while Finufft has int64_t. Since we're releasing a new major version, this is fine, but if we leave it, we'd be stuck with int for a while. On the other hand, may not be too big of an issue. Will put together a quick PR to see how difficult it would be.

@janden
Copy link
Collaborator Author

janden commented Dec 26, 2023

Actually, looking through the code, it seems that we're defining CUFINUFFT_BIGINT specifically to be int and doing checks in makeplan to make sure that the int64_t values we're being fed actually fit into int32_ts, so I'm guessing there's a reason for not allowing full int64_ts here. Leaving this for now.

@blackwer
Copy link
Member

we should make it match finufft and do the check, i think. the reason we have to limit to int32 is that's the limit for cufft

@janden
Copy link
Collaborator Author

janden commented Dec 26, 2023

Ok sounds good. Will take a look tomorrow.

@janden
Copy link
Collaborator Author

janden commented Dec 27, 2023

@blackwer See #411.

@janden
Copy link
Collaborator Author

janden commented Jun 25, 2024

Closed by #411.

@janden janden closed this as completed Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants