inducer / arraycontext

Choose your favorite numpy-workalike!
6 stars 11 forks source link

Added check for ignoring strides of 0 sized arrays #175

Closed mitkotak closed 2 years ago

mitkotak commented 2 years ago

Strides for zero-sized arrays are ill-defined in numpy

import numpy as np
a = np.array([])
a.strides # prints (8,)
alexfikl commented 2 years ago

Why is this needed now? We fixed a bunch of these issues upstream in pyopencl and loopy and there's a test that was passing now for everyone at https://github.com/inducer/arraycontext/blob/d1157d1aed6e9a2c713714c656843c260e7bfa48/test/test_arraycontext.py#L1001

kaushikcfd commented 2 years ago

Why is this needed now? We fixed a bunch of these issues upstream in pyopencl and loopy and there's a test that was passing now for everyone at

Hmm.. I thought checking strides on 0-size arrays didn't make sense as the elements cannot be accessed, so the strides are free to take any value.

As a side note, PyCUDA arrays don't respect NumPy's strides for 0-sized arrays and I didn't quite think PyCUDA was at fault as semantically everything is still correct.

import numpy as np
import pycuda.autoinit  # noqa: F401
import pycuda.gpuarray as cu_np

a = np.zeros((10, 0))
a_gpu = cu_np.to_gpu(a)
print(a.strides)        # prints: (8, 8)
print(a_gpu.strides)    # prints: (0, 8)

Trying this on current pycuda@main will run into a pycuda bug which can be avoided via the following patch--

diff --git a/pycuda/gpuarray.py b/pycuda/gpuarray.py
index a0bf84c..c19810d 100644
--- a/pycuda/gpuarray.py
+++ b/pycuda/gpuarray.py
@@ -916,7 +916,7 @@ class GPUArray:
             dtype=self.dtype,
             allocator=self.allocator,
             base=self,
-            gpudata=int(self.gpudata),
+            gpudata=int(self.gpudata) if self.gpudata is not None else 0,
             order=order,
         )

There are plans to upstream this to pycuda soon.

alexfikl commented 2 years ago

Hmm.. I thought checking strides on 0-size arrays didn't make sense as the elements cannot be accessed, so the strides are free to take any value.

From what I remember, numpy ignores strides when looking at contiguity if the array has size 0. For access it seems like it still works with ary[i, ()] for a size of (n, 0), but the strides are also pretty much ignored.

print(a.strides)        # prints: (8, 8)
print(a_gpu.strides)    # prints: (0, 8)

Yeah, pyopencl used to do this too and was changed to match numpy a bit better in inducer/pyopencl#514. It still raises an index error on ary[i, ()] though.

Either way, I didn't know this was for pycuda, so was just curious if something broke!

inducer commented 2 years ago

Thanks for working on this! This change is fine by me, I'd be happy to merge it (pending the small fix above).

Like @alexfikl, I'm a bit curious what brought about the impulse to make this change.

mitkotak commented 2 years ago

My CI for PyCUDArrayContext was failing due to the test case mentioned above which led me to investigate 0 sized arrays

Why is this needed now? We fixed a bunch of these issues upstream in pyopencl and loopy and there's a test that was passing now for everyone at

https://github.com/inducer/arraycontext/blob/d1157d1aed6e9a2c713714c656843c260e7bfa48/test/test_arraycontext.py#L1001

inducer commented 2 years ago

Thanks!