-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[jit] Fix broken indexing when using None and ellipses indexing together #22905
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
t-vi
left a comment
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.
To me this looks good. Thank you @Chillee!
I wonder whether one could comment the code a bit more (that we're doing forward and backward and split at the ellipse or so, ...), maybe also mark the end of the handle_indexing.
|
@t-vi I added some comments explaining what's going on (maybe too much comments...) |
t-vi
left a comment
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.
Looks good to me, thank you!
| indices = [random.choice(vals) for _ in range(4)] | ||
| indices[random.randint(0, len(indices) - 1)] = "..." | ||
| test_str = dedent(""" | ||
| def f(): |
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.
Can we just use ScriptModule.define() here instead of exec?
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.
I suppose we could, but we use execWrapper everywhere else in our tests as well, and it would require us to wrap it in a module.
Is there any particular reason to prefer ScriptModule.define()?
facebook-github-bot
left a comment
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.
@Chillee is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
#20153
I believe you need 2 passes for this. Take this example
which results in
[10, 9, 8, 7, 6, 1, 1]vs
which results in
[10, 9, 8, 7, 1, 1, 6]After only processing
x[..., None, Nonewe don't know whether we should be creating a new dimension at the end of the dimension list or somewhere in the middle. What we do depends on the elements to the right of it.Thus, I do 2 passes - one to collect all the dimensions that the index operations operate on, and another that executes the index operations.
This still doesn't work for an ellipse index followed by a tensor index, but it wasn't working previously either.