Skip to content
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

Fix ONNX-Chainer's GetItem converter to handle -1 correctly #8460

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

msakai
Copy link
Contributor

@msakai msakai commented Nov 19, 2019

When -1 is given as a slice, current implementation wrongly treat it as slice(-1, 0) which select no elements at all.

This PR avoid the problem by treating it as slice(-1, np.iinfo(np.int64).max) because ONNX's Operator Schemas recommends to pass in INT_MAX for slicing to the end of a dimension with unknown size.

When -1 is given as a slice, current implementation wrongly treat it
as slice(-1, 0) which select no elements at all. We avoid the problem
by treating it as slice(-1, np.iinfo(np.int64).max) because ONNX's
Operator Schemas recommends to pass in INT_MAX for slicing to the end
of a dimension with unknown size.
@emcastillo emcastillo added the ONNX Related to ONNX label Nov 20, 2019
@disktnk
Copy link
Member

disktnk commented Nov 20, 2019

Jenkins, test this please!

@disktnk disktnk self-requested a review November 20, 2019 08:26
@disktnk disktnk self-assigned this Nov 20, 2019
Copy link
Member

@disktnk disktnk left a comment

Choose a reason for hiding this comment

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

LGTM

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 899381b, target branch master) succeeded!

@disktnk
Copy link
Member

disktnk commented Nov 21, 2019

The travis fail is unrelated to this PR, so merge this manually. (The error has already resolved by another PR).

@disktnk disktnk merged commit c941aa9 into chainer:master Nov 21, 2019
@kmaehashi kmaehashi added this to the v7.0.0 milestone Dec 5, 2019
@kmaehashi kmaehashi added the cat:bug Bug report or fix. label Dec 5, 2019
@emcastillo emcastillo changed the title Fix ONNX-Chainer's GetItem converter to handle -1 correctly Fix ONNX-Chainer's GetItem converter to handle -1 correctly Dec 5, 2019
@emcastillo emcastillo changed the title Fix ONNX-Chainer's GetItem converter to handle -1 correctly Fix ONNX-Chainer's GetItem converter to handle -1 correctly Dec 5, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 899381b, target branch master) succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:bug Bug report or fix. ONNX Related to ONNX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants