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.

Sign up for GitHub

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

Jump to bottom

Fix #8477: Allow copies with different strides for 0-length data #8482

Merged
merged 3 commits into from Oct 11, 2022
Merged

Fix #8477: Allow copies with different strides for 0-length data #8482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23c8934
Fix #8477: Allow copies with different strides for 0-length data
gmarkall Oct 3, 2022
a3716cd
CUDA: Test copies in all directions for #8477 fix
gmarkall Oct 3, 2022
9546e58
CUDA: Skip test for #8477 fix in cudasim
gmarkall Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion numba/cuda/cudadrv/devicearray.py
View file
Open in desktop
Expand Up @@ -895,6 +895,8 @@ def check_array_compatibility(ary1, ary2):
if ary1sq.shape != ary2sq.shape:
raise ValueError('incompatible shape: %s vs. %s' %
(ary1.shape, ary2.shape))
if ary1sq.strides != ary2sq.strides:
# We check strides only if the size is nonzero, because strides are
# irrelevant (and can differ) for zero-length copies.
if ary1.size and ary1sq.strides != ary2sq.strides:
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: cases like:

ary1 = np.ndarray(shape=(1, 1, 1), strides=(0, 2, 4), dtype=np.float64)
ary2 = np.ndarray(shape=(), strides=(), dtype=np.float64)

are "safe" due to the squeeze above. It shouldn't be possible to reach here and need to check ary2.size.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also checked manually with:

from numba import cuda
import numpy as np

x  = np.ndarray(shape = (1, 1, 1), strides = (0, 2, 4), dtype=np.float64)
y  = np.ndarray(shape = (), strides = (), dtype=np.float64)

print(x.shape, x.strides, x.size)
print(y.shape, y.strides, y.size)

dx = cuda.to_device(x)
dy = cuda.to_device(y)

dx.copy_to_host(y)
dy.copy_to_host(x)

without issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks for checking this too!

raise ValueError('incompatible strides: %s vs. %s' %
(ary1.strides, ary2.strides))
35 changes: 35 additions & 0 deletions numba/cuda/tests/cudadrv/test_cuda_ndarray.py
View file
Open in desktop
Expand Up @@ -420,6 +420,41 @@ def test_bug6697(self):
got = np.asarray(dary)
self.assertEqual(got.dtype, dary.dtype)

@skip_on_cudasim('DeviceNDArray class not present in simulator')
def test_issue_8477(self):
# Ensure that we can copy a zero-length device array to a zero-length
# host array when the strides of the device and host arrays differ -
# this should be possible because the strides are irrelevant when the
# length is zero. For more info see
# https://github.com/numba/numba/issues/8477.

# Create a device array with shape (0,) and strides (8,)
dev_array = devicearray.DeviceNDArray(shape=(0,), strides=(8,),
dtype=np.int8)

# Create a host array with shape (0,) and strides (0,)
host_array = np.ndarray(shape=(0,), strides=(0,), dtype=np.int8)

# Sanity check for this test - ensure our destination has the strides
# we expect, because strides can be ignored in some cases by the
# ndarray constructor - checking here ensures that we haven't failed to
# account for unexpected behaviour across different versions of NumPy
self.assertEqual(host_array.strides, (0,))

# Ensure that the copy succeeds in both directions
dev_array.copy_to_host(host_array)
dev_array.copy_to_device(host_array)

# Ensure that a device-to-device copy also succeeds when the strides
# differ - one way of doing this is to copy the host array across and
# use that for copies in both directions.
dev_array_from_host = cuda.to_device(host_array)
self.assertEqual(dev_array_from_host.shape, (0,))
self.assertEqual(dev_array_from_host.strides, (0,))

dev_array.copy_to_device(dev_array_from_host)
dev_array_from_host.copy_to_device(dev_array)
Comment on lines +444 to +456
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there's no merit in checking the values of the copies as this is about testing that the copies have succeeded without error?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's no merit - I think there's already tests that copies work, this test is focused on an edge case in checking compatibility. Additionally, since these are empty arrays, there's not a lot to look at to check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming, I'm inclined to agree, but thought it best to check!



class TestRecarray(CUDATestCase):
def test_recarray(self):
Expand Down