docs: document structural_equal and structural_hash for Array and Map #379
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a limitation where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds structural equality and hashing support for Array and Map containers. The changes involve exposing C++ StructuralEqual and StructuralHash functionalities through the FFI to Python, and implementing __eq__, __ne__, and __hash__ methods on the Python Array and Map classes. The implementation is sound and the accompanying tests verify the basic functionality. My review includes suggestions to simplify the Python code by removing redundant __ne__ implementations and to significantly expand the test suite to cover more edge cases and interactions with native Python types, ensuring the new features are robust.
python/tvm_ffi/container.py
Outdated
| def __ne__(self, other: object) -> bool: | ||
| """Return True if the array is not equal to another array.""" | ||
| return not _ffi_api.ArrayEqual(self, other) |
There was a problem hiding this comment.
The __ne__ method can be removed. Python's default implementation for __ne__ delegates to __eq__ and inverts the result, which is exactly what's being done here. Removing this explicit implementation will make the code more concise without changing its behavior.
python/tvm_ffi/container.py
Outdated
| def __ne__(self, other: object) -> bool: | ||
| """Return True if the map is not equal to another map.""" | ||
| return not _ffi_api.MapEqual(self, other) |
There was a problem hiding this comment.
Similar to the Array class, the __ne__ method for Map can be removed. Python will automatically provide the correct __ne__ behavior based on the __eq__ implementation, making this explicit definition redundant.
tests/python/test_container.py
Outdated
| [ | ||
| (tvm_ffi.Array([1, 2, 3]), tvm_ffi.Array([1, 2, 3]), True), | ||
| (tvm_ffi.Array([1, 2, 3]), tvm_ffi.Array([1, 2, 4]), False), | ||
| (tvm_ffi.Array([tvm_ffi.Array([1])]), tvm_ffi.Array([tvm_ffi.Array([1])]), True), | ||
| (tvm_ffi.Array([tvm_ffi.Array([1])]), tvm_ffi.Array([tvm_ffi.Array([2])]), False), | ||
| (tvm_ffi.Map({"a": 1, "b": 2}), tvm_ffi.Map({"a": 1, "b": 2}), True), | ||
| (tvm_ffi.Map({"a": 1, "b": 2}), tvm_ffi.Map({"a": 1, "b": 3}), False), | ||
| ], |
There was a problem hiding this comment.
The current tests for container equality are a good start, but they could be more comprehensive. I suggest adding more test cases to cover comparisons with native Python containers (list, dict), order independence for maps, handling of empty containers, nested containers, and comparisons between different container types. This will ensure the new equality and hashing logic is robust.
[
# Array equality
(tvm_ffi.Array([1, 2, 3]), tvm_ffi.Array([1, 2, 3]), True),
(tvm_ffi.Array([1, 2, 3]), tvm_ffi.Array([1, 2, 4]), False),
(tvm_ffi.Array([1, 2, 3]), [1, 2, 3], True),
(tvm_ffi.Array([1, 2, 3]), [1, 2, 4], False),
(tvm_ffi.Array([]), tvm_ffi.Array([]), True),
(tvm_ffi.Array([]), [], True),
(tvm_ffi.Array([1]), [], False),
# Nested Array equality
(tvm_ffi.Array([tvm_ffi.Array([1])]), tvm_ffi.Array([tvm_ffi.Array([1])]), True),
(tvm_ffi.Array([tvm_ffi.Array([1])]), tvm_ffi.Array([tvm_ffi.Array([2])]), False),
(tvm_ffi.Array([[1]]), [[1]], True),
# Map equality
(tvm_ffi.Map({"a": 1, "b": 2}), tvm_ffi.Map({"a": 1, "b": 2}), True),
(tvm_ffi.Map({"a": 1, "b": 2}), tvm_ffi.Map({"b": 2, "a": 1}), True),
(tvm_ffi.Map({"a": 1, "b": 2}), tvm_ffi.Map({"a": 1, "b": 3}), False),
(tvm_ffi.Map({"a": 1, "b": 2}), {"a": 1, "b": 2}, True),
(tvm_ffi.Map({"a": 1, "b": 2}), {"b": 2, "a": 1}, True),
(tvm_ffi.Map({"a": 1}), tvm_ffi.Map({"c": 1}), False),
(tvm_ffi.Map({"a": 1}), tvm_ffi.Map({"a": 1, "b": 2}), False),
(tvm_ffi.Map({}), tvm_ffi.Map({}), True),
(tvm_ffi.Map({}), {}, True),
(tvm_ffi.Map({1: 2}), {}, False),
# Nested Map equality
(tvm_ffi.Map({"a": {"b": 1}}), tvm_ffi.Map({"a": {"b": 1}}), True),
(tvm_ffi.Map({"a": {"b": 1}}), tvm_ffi.Map({"a": {"b": 2}}), False),
# Cross-type equality
(tvm_ffi.Array([1]), tvm_ffi.Map({1: 1}), False),
(tvm_ffi.Array([]), tvm_ffi.Map({}), False),
],
guan404ming
force-pushed
the
add-array-equality
branch
from
2ee14ac to
3c537e7
Compare
|
I'm not totally sure about using structural equality/hash for array/map comparison - It's a recursively defined equality based on IR structure, which is rather compute intensive. Does Python define it's tuple/dict equality recursively? |
Thanks for the question! Based on my review of CPython's implementation, Python does use recursive equality for tuple/dict. For example, tuple_richcompare (tupleobject.c:689) calls PyObject_RichCompareBool recursively on each element. Dict equality follows the same pattern. So I think Reference
|
guan404ming
force-pushed
the
add-array-equality
branch
from
3756cda to
800a62f
Compare
|
Thanks for the contribution. For this particular case, i feel we should defer a bit, since structural equalty is already supported through explicitly. While classes like list do not necessarily support nested comparison directly via operators. For immutable objects there is some merits for having the feature. This is mainly a consideration of minimalism and also consistency with wrt to instances like c++. As a middle ground, perhaps let us first document structural_equal and structural_hash APIs and refer users to them. Personally i think keeping it simple here might better |
guan404ming
changed the title
|
Thanks for the review and suggestion! I agree that keeping it simple and explicit is the better approach. I've updated the PR to just add documentation pointing users to structural_equal and structural_hash APIs. |
guan404ming
force-pushed
the
add-array-equality
branch
from
800a62f to
ae3007b
Compare
3 participants
Why
Users need guidance on how to perform structural equality comparison for containers.
How