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

An error in the documentation of cl.enqueue_copy #483

Closed
doetools opened this issue Jun 10, 2021 · 3 comments
Closed

An error in the documentation of cl.enqueue_copy #483

doetools opened this issue Jun 10, 2021 · 3 comments

Comments

@doetools
Copy link

doetools commented Jun 10, 2021

In this documentation, I think it should be dst_offset instead of dest_offset, per the input arguments of function _enqueue_copy_buffer in pyopencl/cffi_cl.py, on line 1330.

image

@doetools
Copy link
Author

doetools commented Jun 10, 2021

It turned out I was confused by cl.enqueue_copy and _cl._enqueue_copy_buffer . The input argument of the first uses order of dst_buffer, src_buffer while that of the second uses src_buffer, dst_buffer (I guess the reason is to make it be on par with OpenCL specification of clEnqueueCopyBuffer). The former one uses dest_offset while the latter uses dst_offset as positional argument. In the _init_.py, there is a conversion to pop out dest_offset key and writes a key of dst_offset.

Don't get me wrong, I love using pyopencl (even more than C OpenCL)! It was my feeling that this part took me some effort to figure out. Someone with a C OpenCL background (like me) might come across this situation too.

@inducer
Copy link
Owner

inducer commented Jun 10, 2021

  • eneuque_copy unifies all the copy interfaces, and it adopts C memcpy's (target, source) ordering consistently. That's admittedly different from clEnqueueCopyBuffer, but at this point I'd say that's unlikely to change.
  • I agree that the dest_offset vs dst_offset kwarg naming is a bit unfortunate and inconsistent. I'd be happy to take a patch that deprecates (but still supports!) the old argument name while introducing (and documenting) the new one.

@inducer
Copy link
Owner

inducer commented Sep 16, 2022

#452 has deprecations to standardize on dst_offset.

@inducer inducer closed this as completed Sep 16, 2022
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

No branches or pull requests

2 participants