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 the Cython layer and use ctypes #183

Merged
merged 23 commits into from
Mar 18, 2024
Merged

Remove the Cython layer and use ctypes #183

merged 23 commits into from
Mar 18, 2024

Conversation

paskino
Copy link
Contributor

@paskino paskino commented Mar 11, 2024

Removes the Cython layer in favour of ctypes. My aim has been to generally maintain the user experience however, this change is backward incompatible.

New functionality

In python it is possible now to pass to each function an out parameter for the regulariser to store the result. This was earlier not possible and the output was always allocated in the Cython layer. The output is also always returned, remaining backward compatible.

Backward incompatibilities

infovector

The infovector, a vector of 2 floats with information on whether the algorithm stopped having reached a certain tolerance, is never returned. To access the infovector the user needs to pass the function a preallocated vector with the key-worded parameter infovector.

device

Most of the functions have a OpenMP and CUDA version (CPU and GPU). The user could earlier specify which one to use with the positional parameter device. To remove lots of boilerplate code, device is now a key-worded argument because the C functions are wrapped in Python by the function below and, the positional parameters of each function are different, so I am never sure where device really is.

At any rate I believe this change adds clarity and the default is device="cpu".

https://github.com/vais-ral/CCPi-Regularisation-Toolkit/blob/739e52af5542b2debb94ddfab0bce0901e7f2aae/src/Python/ccpi/filters/regularisers.py#L19-L32

PD_TV positional parameter order

I changed the parameter order of the PD_TV to reflect the underlying C/CUDA implementation.

@paskino paskino self-assigned this Mar 11, 2024
@dkazanc
Copy link
Collaborator

dkazanc commented Mar 11, 2024

thanks @paskino for looking into this. I think dropping Cython in favour of Ctypes is probably for the best here. Happy to contribute at some point, I was thinking of integrating more rigorous unit testing as well.

We are planning to use some CuPy wrapped functions in production from the toolkit, so it is worth looking into how both cupy and cuda parts can be added in one conda build. I can do that later when the build is actually works. cheers

@paskino paskino marked this pull request as ready for review March 15, 2024 12:43
@paskino paskino requested review from casperdcl and dkazanc and removed request for casperdcl March 15, 2024 12:43
@paskino paskino removed their assignment Mar 15, 2024
@casperdcl casperdcl self-assigned this Mar 15, 2024
test/test_run_test.py Outdated Show resolved Hide resolved
test/test_run_test.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
3 participants