gemini-code-assist[bot] commented on code in PR #102:
URL: https://github.com/apache/tvm-ffi/pull/102#discussion_r2423776671
##########
python/tvm_ffi/cython/tensor.pxi:
##########
@@ -179,6 +179,18 @@ def _shape_obj_get_py_tuple(obj):
return tuple(shape.data[i] for i in range(shape.size))
+def _make_strides_from_shape(tuple shape):
+ cdef int64_t expected_stride = 1
+ cdef list strides = []
+ cdef int64_t ndim = len(shape)
+ cdef int64_t reverse_index
+ for i in range(ndim):
+ reverse_index = ndim - i - 1
+ strides.append(expected_stride)
+ expected_stride *= shape[reverse_index]
+ return tuple(reversed(strides))
Review Comment:
The implementation of `_make_strides_from_shape` is functionally correct,
but it could be made more direct and easier to read. It currently builds a list
of strides in reverse order and then reverses the list. A more straightforward
approach would be to pre-allocate the list and fill in the strides from right
to left. This avoids the `reversed()` call and the temporary `reverse_index`
variable, improving maintainability.
```
def _make_strides_from_shape(tuple shape):
cdef int64_t ndim = len(shape)
if ndim == 0:
return ()
cdef list strides = [0] * ndim
strides[ndim - 1] = 1
cdef int64_t i
for i in range(ndim - 2, -1, -1):
strides[i] = strides[i + 1] * shape[i + 1]
return tuple(strides)
```
##########
tests/python/test_tensor.py:
##########
@@ -38,6 +38,7 @@ def test_tensor_attributes() -> None:
x = tvm_ffi.from_dlpack(data)
assert isinstance(x, tvm_ffi.Tensor)
assert x.shape == (10, 8, 4, 2)
+ assert x.strides == (64, 8, 2, 1)
Review Comment:
The current test validates the `strides` property for a contiguous tensor,
which is a great start. To ensure the implementation is robust, it would be
beneficial to also test the case where strides are provided by the DLPack
tensor (i.e., `self.cdltensor.strides != NULL`). This can be achieved by
creating a non-contiguous NumPy array (e.g., by transposing an array or using
slicing with a step) and verifying that `tvm_ffi.Tensor` correctly reports its
strides. This would cover the `else` branch in the `strides` property's
implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
