gemini-code-assist[bot] commented on code in PR #45:
URL: https://github.com/apache/tvm-ffi/pull/45#discussion_r2371149875
##########
python/tvm_ffi/container.py:
##########
@@ -143,6 +143,14 @@ def __repr__(self) -> str:
return type(self).__name__ + "(chandle=None)"
return "[" + ", ".join([x.__repr__() for x in self]) + "]"
+ def __add__(self, other: Sequence[T]) -> Array[T]:
+ """Concatenate two arrays."""
+ return Array[T](tuple(self) + tuple(other))
Review Comment:
Using `Array[T]` as a constructor will raise a `TypeError` at runtime. You
should use `type(self)` to correctly instantiate a new array, which also
supports potential subclasses.
Additionally, the current implementation creates intermediate tuples for
both `self` and `other` before concatenating them, which is inefficient for
large arrays. You can use `itertools.chain` to create a single iterator over
both sequences and then convert it to a tuple for the constructor. This is more
memory-efficient.
You'll need to add `import itertools` at the top of the file.
```suggestion
return type(self)(tuple(itertools.chain(self, other)))
```
##########
python/tvm_ffi/container.py:
##########
@@ -143,6 +143,14 @@ def __repr__(self) -> str:
return type(self).__name__ + "(chandle=None)"
return "[" + ", ".join([x.__repr__() for x in self]) + "]"
+ def __add__(self, other: Sequence[T]) -> Array[T]:
+ """Concatenate two arrays."""
+ return Array[T](tuple(self) + tuple(other))
+
+ def __radd__(self, other: Sequence[T]) -> Array[T]:
+ """Concatenate two arrays."""
+ return Array[T](tuple(other) + tuple(self))
Review Comment:
Similar to `__add__`, using `Array[T]` as a constructor will fail at
runtime. Please use `type(self)` instead.
For efficiency, `itertools.chain` should be used here as well to avoid
creating unnecessary intermediate tuples.
```suggestion
return type(self)(tuple(itertools.chain(other, self)))
```
##########
tests/python/test_container.py:
##########
@@ -124,3 +125,34 @@ def test_serialization() -> None:
a = tvm_ffi.convert([1, 2, 3])
b = pickle.loads(pickle.dumps(a))
assert str(b) == "[1, 2, 3]"
+
+
[email protected](
+ "a, b, c_expected",
+ [
+ (
+ tvm_ffi.Array([1, 2, 3]),
+ tvm_ffi.Array([4, 5, 6]),
+ tvm_ffi.Array([1, 2, 3, 4, 5, 6]),
+ ),
+ (
+ tvm_ffi.Array([1, 2, 3]),
+ [4, 5, 6],
+ tvm_ffi.Array([1, 2, 3, 4, 5, 6]),
+ ),
+ (
+ [1, 2, 3],
+ tvm_ffi.Array([4, 5, 6]),
+ tvm_ffi.Array([1, 2, 3, 4, 5, 6]),
+ ),
+ ],
Review Comment:
The test cases are good. To make them more comprehensive, please consider
adding cases that involve empty arrays or lists to test edge cases.
```python
[
(
tvm_ffi.Array([1, 2, 3]),
tvm_ffi.Array([4, 5, 6]),
tvm_ffi.Array([1, 2, 3, 4, 5, 6]),
),
(
tvm_ffi.Array([1, 2, 3]),
[4, 5, 6],
tvm_ffi.Array([1, 2, 3, 4, 5, 6]),
),
(
[1, 2, 3],
tvm_ffi.Array([4, 5, 6]),
tvm_ffi.Array([1, 2, 3, 4, 5, 6]),
),
(
tvm_ffi.Array([]),
tvm_ffi.Array([1, 2, 3]),
tvm_ffi.Array([1, 2, 3]),
),
(
tvm_ffi.Array([1, 2, 3]),
[],
tvm_ffi.Array([1, 2, 3]),
),
(
[],
tvm_ffi.Array([1, 2, 3]),
tvm_ffi.Array([1, 2, 3]),
),
(
tvm_ffi.Array([]),
[],
tvm_ffi.Array([]),
),
],
```
--
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]
