Add support for spreadinterponly in finufft
#599
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.
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.
I wanted to use std::reference_wrapper, but I think that still wont work.
There was a problem hiding this comment.
Why do you need to create a vector from a pointer?
There was a problem hiding this comment.
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.
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.
We aim to remove all pointers ad c++ data structures are safer. Pointers can cause memory leaks
There was a problem hiding this comment.
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.
In fact reference wrapper is cleaner and way less hacky than what i have now
There was a problem hiding this comment.
Did you remove all of this? I cannot find the hacky code anymore
There was a problem hiding this comment.
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. |
This resolves #598 just like as done in #564 for cufinufft.