-
Notifications
You must be signed in to change notification settings - Fork 50
fix(Array): Make array slicing behavior to return list #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @junrushao, 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 resolves a backward compatibility regression related to the slicing behavior of Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes a backward compatibility issue where slicing a tvm_ffi.Array was returning an Array instead of a list. The change correctly reverts this behavior. The added test case also properly verifies the fix. My review includes one suggestion to update the type hints and docstring in Array.__getitem__ to be consistent with the new behavior, improving code clarity and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a backward compatibility issue by ensuring that slicing a tvm_ffi.Array returns a list instead of an Array. The changes in python/tvm_ffi/container.py correctly revert the slicing behavior, and the accompanying test in tests/python/test_container.py validates this fix. My review includes a suggestion to refine the type hints in __getitem__ to more accurately reflect the returned types, which will improve code clarity and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a backward compatibility issue by ensuring that slicing a tvm_ffi.Array returns a list instead of an Array. The changes in python/tvm_ffi/container.py correctly implement this behavior, and the update in tests/python/test_container.py adds a necessary check to validate it. My review includes one point of feedback to improve code documentation and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a backward compatibility issue by ensuring that slicing a tvm_ffi.Array returns a list instead of another Array. The changes in python/tvm_ffi/container.py correctly adjust the __getitem__ method's implementation, type hints, and docstring to reflect this behavior. The accompanying update to tests/python/test_container.py adds a crucial assertion to verify the returned type, which helps prevent future regressions. I've added one suggestion to make the test even more robust. Overall, this is a good and necessary fix.
This PR fixes a behavioral inconsistency between existing TVM stack and the latest
main.Originally, slicing a
tvm_ffi.Array[T]always returnslist[T]. This behavior was altered by PR #37, which instead returnsArray[T], breaking a few unittests.