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

Remove dim and type from cufinufft_default_opts. #337

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

janden
Copy link
Collaborator

@janden janden commented Aug 31, 2023

This brings the interface in line with Finufft per #255. The dim and type (well, actually only type) were used to set gpu_method. Now this is set to 0, an “auto” setting which results in make_plan choosing the most appropriate method for the type. (We'll be able to incorporate precision later as well, see #321.)

Resolves #255.

Now only requires a pointer to the options struct since we don't need
the type or dimension anymore.
@janden janden marked this pull request as ready for review August 31, 2023 08:53
@janden
Copy link
Collaborator Author

janden commented Aug 31, 2023

@blackwer No rush. Just figured you'd want to look through this when you get back.

Copy link
Member

@blackwer blackwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some basic formatting changes to match the .clang-format but otherwise it looks good to me

@@ -104,54 +103,11 @@ int cufinufft_default_opts(int type, int dim, cufinufft_opts *opts)

opts->gpu_maxbatchsize = 0; // Heuristically set

switch (dim) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does some sanity checking, which is now gone. This defers it to a later step, which is fine, but we should consider how to best deal with invalid inputs at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'll move it into makeplan at some point. Will make an issue.

@blackwer blackwer merged commit cf516fb into flatironinstitute:master Sep 8, 2023
8 checks passed
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 this pull request may close these issues.

finufft/cufinufft interface rationalization (C)
2 participants