gemini-code-assist[bot] commented on code in PR #45:
URL: https://github.com/apache/tvm-ffi/pull/45#discussion_r2371187368
##########
tests/python/test_container.py:
##########
@@ -124,3 +125,54 @@ 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]),
+ ),
+ (
+ 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([]),
+ ),
Review Comment:
The test suite provides good coverage for `Array` and `list` operands. To
make it more comprehensive and ensure broad compatibility with Python's
built-in sequence types as mentioned in the pull request description, I
recommend adding test cases for concatenating with `tuple`s. This will help
verify that `__add__` and `__radd__` behave correctly with other common
sequence types.
```python
(
tvm_ffi.Array([]),
[],
tvm_ffi.Array([]),
),
(
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]),
),
```
##########
python/tvm_ffi/container.py:
##########
@@ -143,6 +144,14 @@ def __repr__(self) -> str:
return type(self).__name__ + "(chandle=None)"
return "[" + ", ".join([x.__repr__() for x in self]) + "]"
+ def __add__(self, other: Iterable[T]) -> Array[T]:
+ """Concatenate two arrays."""
+ return type(self)(itertools.chain(self, other))
+
+ def __radd__(self, other: Iterable[T]) -> Array[T]:
+ """Concatenate two arrays."""
+ return type(self)(itertools.chain(other, self))
Review Comment:
The current implementation of `__add__` and `__radd__` accepts any
`Iterable`. This is very flexible, but can lead to surprising behavior. For
example, concatenating with a `set` would have a non-deterministic order, and
concatenating with a `str` would add individual characters. Python's built-in
`list.__add__` is much stricter, only accepting other lists, to avoid such
ambiguities.
To ensure predictable, ordered concatenation, which is what users typically
expect from the `+` operator on sequence-like types, I suggest restricting the
`other` operand to be a `Sequence` (but not `str` or `bytes`). This would align
better with the behavior of Python's built-in sequences and prevent unexpected
outcomes.
```suggestion
def __add__(self, other: Sequence[T]) -> "Array[T]":
"""Concatenate two arrays."""
if not isinstance(other, Sequence) or isinstance(other, (str,
bytes)):
return NotImplemented
return type(self)(itertools.chain(self, other))
def __radd__(self, other: Sequence[T]) -> "Array[T]":
"""Concatenate two arrays."""
if not isinstance(other, Sequence) or isinstance(other, (str,
bytes)):
return NotImplemented
return type(self)(itertools.chain(other, self))
```
--
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]
