-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding advanced interface #68
Conversation
@lgarrison — Here's my first pass at all this. Here's how we would provide the configuration relevant for #67: from jax_finufft import nufft2, options
opts = options.NestedOpts(
type1=options.Opts(gpu_method=1),
type2=options.Opts(gpu_method=2),
)
nufft2(..., opts=opts) or, equivalently in this case: from jax_finufft import nufft2, options
opts = options.NestedOpts(
forward=options.Opts(gpu_method=2),
backward=options.Opts(gpu_method=1),
)
nufft2(..., opts=opts) It's not all that ergonomic, but I think it's a decent start! |
I've also done something with the imports to break the CUDA compilation. I think it has something to do with |
The immediate CUDA compilation error is just that there's no declaration of the Some CUDA tests fail locally with a CUDA illegal memory access. Not yet sure if it's a problem with the opts, or this header refactoring. |
The problem was indeed with the header refactoring. The I don't like my solution, it feels fragile to me! It's too easy to write a specialization in the source file that gets silently ignored. Not sure if there's a better pattern we should be using here. |
Thanks for taking this down @lgarrison!! I'll take a look this afternoon. |
I confirm the opts are working for me and fix the performance issue from #67. |
Thanks @lgarrison! I think that the approach you came up with here is totally fine. I agree that it's not very elegant, but I think we should just roll with it and revisit only if we need to later. It's possible that the whole library could benefit from some refactoring, but let's not let that get in the way of merging this. With that in mind, I'm going to merge this now! This fixes #67, but let's leave #66 open until we add info to the README. |
This is still very much a work in progress, but it should help with #66 and #67. I'll request feedback ASAP!