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

Floating point conversion in Cython wrapper. #515

Merged
merged 5 commits into from
Nov 10, 2023

Conversation

vathomass
Copy link
Contributor

This pull request is related to issue #509.

There are three changes:

  1. Proper conversion of the floating point arguments to cl_half. This will preserve the fp value for the calculations. I see three ways that this can be done: a) use the native opencl header cl_half.h which provides functions for conversion, b) use the clblast machinery for converting floats to halfs or c) use the numpy core library to do the conversion. My preference is option c) and this is the one I have implemented, as it does not add extra dependencies to the module.
  2. Using generator.py script in windows add CRLF line endings, which git picks up as changes. For convenience, the line endings have been forced to be LF, independent of the platform.
  3. Some routines expect an integer output (which is passed as pyopencl array class in the python implementation), like amax and amin. The cython wrapper expects these arguments as floats with the same accuracy as the other vector inputs. In the C side of the code these arguments are of type size_t. Python users have to convert the floating point values to integers to recover the value, by for example calling np.frombuffer(val.get().tobytes(), np.uint32)[0]. Moreover, with the fp16 these function error out with RuntimeError: PyCLBlast: 'CLBlastXamax' failed: CLBlastInsufficientMemoryScalar: The unit-sized vector's OpenCL buffer is too small. Changes in the generator script force these arguments to be unsigned integers with different size than the other routine inputs.

This example demonstrates the changes:

import numpy as np
import pyopencl as cl
from pyopencl.array import Array
import pyclblast

# Settings for this sample
dtype = np.float16
alpha = -55
n = 40

print("# Setting up OpenCL")
ctx = cl.create_some_context()
queue = cl.CommandQueue(ctx)

print("# Setting up Numpy arrays")
x = np.random.rand(n).astype(dtype=dtype)
y = np.random.rand(n).astype(dtype=dtype)

print("# Setting up OpenCL arrays")
clx = Array(queue, x.shape, x.dtype)
cly = Array(queue, y.shape, y.dtype)
clx.set(x)
cly.set(y)

print("# Example level-1 operation: AXPY")
pyclblast.axpy(queue, n, clx, cly, alpha=alpha)
print("# Result for vector y: %s" % cly.get())
print("# Expected result:     %s" % (alpha * x + y))

idx = Array(queue, (1, 1), np.uint32)
pyclblast.amax(queue, n=n, x=clx, imax=idx)
print("# Result for abs max:  %s" % idx.get()[0, 0])
print("# Expected result:     %s" % np.argmax(np.abs(x)))

@CNugteren CNugteren self-requested a review November 8, 2023 18:52
Copy link
Owner

@CNugteren CNugteren left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution, and quite brave that you actually dug into my unreadable generator Python mess :-) Everything looks good, but just two small requests:

  1. Could you also update the CHANGELOG file and add 3 short entries that describe the three changes that you've made?
  2. After this is merged I'll make a new release of the python bindings, so perhaps you can also already change version="1.3.2" into version="1.4.0"? Thanks!

@vathomass
Copy link
Contributor Author

Hi,

Thanks for reviewing the code, happy to contribute. I have made the requested changes.

There is another issue with the python package, which I did not wanted to add to this PR, not to complicate things.
Pyclblast does not install on windows properly (it cannot find the clblast.dll, because windows ...). I have made some changes to trick it to work for me, but are not ready for PR (I have hard coded the paths for my PC). If I find time I will open a separate PR, but my time is limited for the next week. If you want to wait a week or two, it is OK, if not, it is OK.

Best
Thomas

CHANGELOG Outdated Show resolved Hide resolved
@CNugteren CNugteren enabled auto-merge (squash) November 10, 2023 19:09
@CNugteren
Copy link
Owner

Thanks for making the changes, I'll merge this in.

f you want to wait a week or two, it is OK, if not, it is OK.

Take your time, I'll wait till the next PR before I make a new pyclblast release.

@CNugteren CNugteren merged commit 564629c into CNugteren:master Nov 10, 2023
4 checks passed
@vathomass vathomass deleted the cython_fp16 branch December 9, 2023 12:28
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.

2 participants