Skip to content

Conversation

@Chillee
Copy link
Collaborator

@Chillee Chillee commented Jul 16, 2019

#20153

I believe you need 2 passes for this. Take this example

@torch.jit.script
def f():
    x = torch.ones(10, 9, 8, 7, 6)
    return x[..., None, None].shape

which results in [10, 9, 8, 7, 6, 1, 1]
vs

@torch.jit.script
def f():
    x = torch.ones(10, 9, 8, 7, 6)
    return x[..., None, None, :].shape

which results in [10, 9, 8, 7, 1, 1, 6]
After only processing x[..., None, None we 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.

@Chillee Chillee requested review from jamesr66a, suo and t-vi July 16, 2019 08:40
@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jul 16, 2019
Copy link
Collaborator

@t-vi t-vi left a 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.

@Chillee
Copy link
Collaborator Author

Chillee commented Jul 17, 2019

@t-vi I added some comments explaining what's going on (maybe too much comments...)

Copy link
Collaborator

@t-vi t-vi left a 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():
Copy link
Collaborator

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?

Copy link
Collaborator Author

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()?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@facebook-github-bot
Copy link
Contributor

@Chillee merged this pull request in a24f6c1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants