Skip to content

Conversation

@padreofthegame
Copy link
Contributor

@padreofthegame padreofthegame commented Jan 23, 2023

Suppose to solve issue #12922.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 23, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@shingjan
Copy link

Thanks for taking a look at this @padreofthegame! Will circle back once this PR is ready for review

@padreofthegame
Copy link
Contributor Author

This solution is very similar to #13306, and represents workaround for the problem mentioned in #12922 for PyTorch models (since advanced indexing does not work the same for example for numpy and for pytorch, I put solution in torch frontend, because issue was reported for torch models).
Also, I put WIP label only because I am not sure if I managed to cover all possible test cases, so @shingjan you are welcome to try some additional scenarios, and call me out if detect something strange.

@shingjan
Copy link

@padreofthegame i think this fix is fine in terms of the difference between adv_index of numpy and it of pytorch. There is no numpy frontend for TVM yet. I think overall this PR LGTM. Will be great if @masahi can take a look and approve. Thanks!

@padreofthegame padreofthegame changed the title WIP: [Torch] Fix advanced indexing with NoneType index arguments [Torch] Fix advanced indexing with NoneType index arguments Jan 27, 2023
Copy link

@shingjan shingjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@masahi masahi merged commit 7aecc1a into apache:main Feb 2, 2023
@padreofthegame padreofthegame deleted the pytorch/adv_index branch February 2, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants