Skip to content

Conversation

@BowenBao
Copy link
Collaborator

This could serve as a alternative solution to export torch.gather before something similar goes into ONNX spec. The exported model is verified to be correct against onnxruntime backend. We weren't able to test against Caffe2 backend because it doesn't seem to support OneHot opset9.

@pytorchbot pytorchbot added the module: onnx Related to torch.onnx label May 31, 2019
@houseroad houseroad self-requested a review June 3, 2019 22:48
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.

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.


@parse_args('v', 'i', 'v', 'v')
def gather(g, self, dim, index, sparse_grad=False):
# NOTE: Update this workaround if ONNX has native Gather support.

Choose a reason for hiding this comment

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

Maybe rephrase to say that current onnx::Gather does not directly support torch.Gather.



@parse_args('v', 'i', 'v', 'v')
def gather(g, self, dim, index, sparse_grad=False):

Choose a reason for hiding this comment

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

This should also support Opset 10 export, correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so. Should be able to fall back to opset 9's symoblics.

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.

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@houseroad
Copy link
Member

@pytorchbot retest this please

@facebook-github-bot
Copy link
Contributor

@houseroad merged this pull request in 63a55d4.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants