Skip to content

Fix bugs in handling of negative slice + gather indices#10973

Closed
jamesr66a wants to merge 1 commit intopytorch:masterfrom
jamesr66a:export-D9546183
Closed

Fix bugs in handling of negative slice + gather indices#10973
jamesr66a wants to merge 1 commit intopytorch:masterfrom
jamesr66a:export-D9546183

Conversation

@jamesr66a
Copy link
Collaborator

Summary: This fixes multiple bugs in the handling of negative indices in both slicing and gather operations. These were uncovered by @[1466077526:Elias Ellison]'s diff D9493614, which made it so that we actually emit negative indices when we see them in PyTorch code.

Differential Revision: D9546183

Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

LG, could you also add some test cases in test_pytorch_onnx_caffe2.py? Also update the doc https://github.com/pytorch/pytorch/blob/master/caffe2/python/onnx/ONNXOpCoverage.md

@houseroad
Copy link
Member

expect files need to be updated. Also we need to make sure that it works on the cases which axis is higher than 2 (according to the doc, c2 gather has problem with higher axis)

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.

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

Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Can we check the axis, if it 0 or 1, we emit Gather, otherwise, we emit Sliceand Squeeze.

This comment was marked as off-topic.

This comment was marked as off-topic.

@jamesr66a jamesr66a force-pushed the export-D9546183 branch 2 times, most recently from ccaab3d to 39359c6 Compare August 29, 2018 03:03
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.

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

@houseroad
Copy link
Member

@jamesr66a thanks for adding the fall back :-) agree, best solution is extending caffe2's kernels.

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.

jamesr66a is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary:
This fixes multiple bugs in the handling of negative indices in both slicing and gather operations. These were uncovered by @[1466077526:Elias Ellison]'s diff D9493614, which made it so that we actually emit negative indices when we see them in PyTorch code.
Pull Request resolved: pytorch#10973

Reviewed By: jhcross

Differential Revision: D9546183

Pulled By: jhcross

fbshipit-source-id: 32b0362eee757e67c5a09144131b1d4eadb9f760
PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
Summary:
This fixes multiple bugs in the handling of negative indices in both slicing and gather operations. These were uncovered by @[1466077526:Elias Ellison]'s diff D9493614, which made it so that we actually emit negative indices when we see them in PyTorch code.
Pull Request resolved: pytorch#10973

Reviewed By: jhcross

Differential Revision: D9546183

Pulled By: jamesr66a

fbshipit-source-id: 6cb0e84e8ad399e47e24a96c44025f644c17b375
@ezyang ezyang added the merged label Jun 26, 2019
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