-
Notifications
You must be signed in to change notification settings - Fork 245
Added NotImplemented error to Array.concatenate #508
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
.. versionadded:: 2013.1 | ||
""" | ||
# {{{ find properties of result array | ||
|
||
shape = None | ||
if axis != 0: |
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.
This isn't correct. This should instead be based on contiguity. Column-major arrays for example allow concatenation along the last axis.
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 noticed that the code failed the tests because I missed a syntax error. Sorry for the noise.
About contiguity, I did some tests, and the function does not work in any axis for colum-major arrays. In line 2571 of the Array.concatenate function, the output array result
is always a row-major array, leading to the same NotImlementedError
due to miss-matching strides.
Simple example:
import pyopencl as cl
platform = cl.get_platforms()[0]
device = platform.get_devices()[0]
context = cl.create_some_context([device])
queue = cl.CommandQueue(context)
a = np.random.rand(2,2)
b = np.random.rand(2,2)
a_cl = clarray.to_device(queue, np.require(a, np.float64, 'F'))
b_cl = clarray.to_device(queue, np.require(b, np.float64, 'F'))
c_cl = clarray.concatenate( (a_cl, b_cl), axis=1)
It shouldn't be hard to allow column-major arrays, it is just a matter of initializing the result array with order="F"
in these cases. The issue I see is that the Array class has no member that indicates the ordering. One could deduce the order by looking at the strides (if Array.strides[0] > Array.strides[-1]: row-major, else: column-major) but maybe it is too hacky. what do you think?
Another approach is the one taken in the Array.stack function, in which when the axis != 0, it is assumed that the input is column mayor. But it would also lead to unclear error messages.
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.
Thanks for pointing out the issue with concatenate
and column-major arrays, we should look into fixing that. I agree with your assessment about inspection of the strides, it's not quite elegant.
FWIW, CI is still not passing on this branch.
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.
Oh yeah! I forgot to push. It should work now.
As suggested in this issue, a warning was added both in the code and in the documentation.