-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[ONNX] Update export for topk and sort #25739
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
@pytorchbot rebase this please |
@pytorchbot retest this please |
1 similar comment
@pytorchbot retest this please |
@pytorchbot rebase this please |
@pytorchbot rebase this please |
@houseroad please review as this op is requested |
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…oof/topk # Conflicts: # torch/onnx/symbolic_opset11.py
@pytorchbot retest this please |
@pytorchbot rebase this please |
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
cc @houseroad for review |
@pytorchbot rebase this please |
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@neginraoof update the expect files please |
@houseroad done, thanks |
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
LGTM.
start = g.op("Constant", value_t=torch.tensor(dim, dtype=torch.int64)) | ||
end = g.op("Constant", value_t=torch.tensor(dim + 1, dtype=torch.int64)) | ||
slice_ = _slice_helper(g, shape_, axes=axis, starts=start, ends=end, steps=None, dynamic_slice=True) | ||
if _export_onnx_opset_version <= 10: |
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.
Ideally my preference is to not capture the entire implementation in a helper function like this, especially when the implementation differs between two opsets like in this case. We should write the implementation in the opset's respective symbolic_opset*. We can capture the common parts of the implementation in a helper function.
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.
Yeah, makes sense
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.
LGTM
@houseroad merged this pull request in d93fc64. |
Summary: updated export for topk and sort as part of opset11 Pull Request resolved: pytorch#25739 Reviewed By: hl475 Differential Revision: D17467131 Pulled By: houseroad fbshipit-source-id: 653be138455728ec8e9bb81ae63dd7ce0c4d0793
Summary: updated export for topk and sort as part of opset11 Pull Request resolved: pytorch#25739 Reviewed By: hl475 Differential Revision: D17467131 Pulled By: houseroad fbshipit-source-id: 653be138455728ec8e9bb81ae63dd7ce0c4d0793
updated export for topk and sort as part of opset11