-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add support for spreadinterponly
in finufft
#599
base: master
Are you sure you want to change the base?
Conversation
I am still thinking on good ways to have tests for it, we will merge only when we have tests up, to prevent instability. |
include/finufft/utils.h
Outdated
// Simple helper to wrap pointer in std::vector and release it later | ||
template <class T> | ||
void wrapArrayInVector( T *sourceArray, size_t arraySize, std::vector<T, xsimd::aligned_allocator<T, 64> > &targetVector ) { | ||
typename std::_Vector_base<T, xsimd::aligned_allocator<T, 64> >::_Vector_impl *vectorPtr = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to rely on some hacky way to move a piointer data to a std::vector, without any copies. Anyone has any ideas on how to make this better? Also, it seems to fail on MAC, but I dont know if std:: changes in mac, I am not used to it. Can someone help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to use std::reference_wrapper, but I think that still wont work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to create a vector from a pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what i need to do is point incoming memory pointer fk to fwbatch, so that the spread / interpolate directly happens on input/ output. In the cufinufft versions, it was fairly straightforward as the memory for fwbatch was not std::vector. Any ideas why you chose it so here?
I see the underlying kernels use pointers anyway exposed through .data().
Another option can be to overload the function and add support for when we send pointer to memory, although the current spread / interpolate doesn't directly take the fwbatch vector as input, but rather the plan itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we template more, so that functions can accepts both pointers and vectors as both have [] operator. Or thsi is a use case for a span. Vector is owning so should not be used thsi way.
We might need to write a simple span implementation since it is available in c++20 onwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aim to remove all pointers ad c++ data structures are safer. Pointers can cause memory leaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about leaks, i have suffered fixing them in gpuNUFFT. Did you happen to use std:: reference_wrapper? I think it's built for this and i somehow am not able to get it work for our use case:
https://en.cppreference.com/w/cpp/utility/functional/reference_wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact reference wrapper is cleaner and way less hacky than what i have now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you remove all of this? I cannot find the hacky code anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup :) No hacks, only clean code :P ... Just changed the function API.
I am quite sure that this is not possible in any portable way. |
@DiamonDinoia , I made some fixes. Please check if this is okay for you . This is a minor change in internal function calls. |
It will take me some time to get back to this. If it is urgent I can assign other reviewers. In the meantime, have you thought of potential tests? |
@DiamonDinoia I dont think its that urgent. In fact, I would prefer to have tests so that this code is more clear. Also, I think there's some work on documentation side still for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm brining it in, but I'm making some tweaks. Also adding docs, and needing to fix up opts in matlab, fortran, etc. I will merge by hand and bring in.
t_sprint += timer.elapsedsec(); | ||
} | ||
// Release the fwBatch vector to prevent double freeing of memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this comment - is there anything to do, given it was not allocated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing with matlab interface, it segfaults...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segfaults fixed - it was the incorrect nf1 = upsampfac * ms, etc. rather nf1 should match user N1 grid size.
However, if you can explain the comment? Since fwBatch is a std::vector field of the plan class, there's no freeing to worry about, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I think this comment was from earlier code and we can remoe this now that I updated the spreadinterpSortedBatch
to directly use the pointer rather than the std::vector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree with you, we dont need to free it manually, as fwBatch is std::vector.
@chaithyagr Could you remind me how ns (spread/interp kernel width, and for that matter, the beta shape param) is supposed to be set by tol here? Is is as if upsampfac=2.0 were set, although there is of course not upsampling anything? That seems to be too restrictive - what if you wanted a kernel appropriate for another upsampfac? I haven't gotten it to work yet - still segfaults with opts.spreadinterponly=1 is set (at least via matlab). Also, is your group/application actively using this option? (I am interested independently, as it provides a simple interface to spread/interp, but the question of setting the width and shape still applies). PS DO NOT MERGE. I have a local branch with many edits. |
@chaithyagr I have a proposal for the interface of spreadinterponly for both CPU and GPU: we interpret (tol, upsampfac) as controlling the kernel width and beta (shape) parameter. Of course there is no upsampling happening, but this is a convenient way to control the kernel (which I certainly will want to do). This differs from the current GPU interface which forces the user to set upsampfac=1.0 but then in fact uses the kernel for upsampfac=2, I believe. (Please answer my prev question about this when you get a chance). Do you agree? I will proceed like this in 2.4.0 for now. |
Devel is moved over to #602 |
This resolves #598 just like as done in #564 for cufinufft.