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

Add support of SelectItem in ONNX-Chainer #8450

Merged
merged 4 commits into from
Nov 20, 2019

Conversation

tkanmae
Copy link
Contributor

@tkanmae tkanmae commented Nov 15, 2019

This Pull Request addresses #8449. The implementation encodes an equivalent of the following NumPy snippet:

row_idxs = np.nonzero(np.ones(t.shape))[0]
n_cols = x.shape[1]
idxs = row_idxs * n_cols + t
output = np.ravel(x)[idxs]

I could have used Range to create row_idxs, but found that some of other ONNX conversion tools out there have not supported it yet. So I settled on the sequence of ConstantOfShape followed by NonZero to create the indices.

I'm not sure if I correctly used support decorator. Among the ops used in the implementation, ConstantOfShape is the newest one introduced in opset 9, and some ops have been updated in opset 11. So I set the argument to the decorator as (9, 11). It will be great if you can take a look at it.

@take-cheeze take-cheeze added the ONNX Related to ONNX label Nov 15, 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.

The alternative logic of F.select_item looks smart! SGTM,

So I set the argument to the decorator as (9, 11). It will be great if you can take a look at it

In this case, support decorator does not have to be set, but I agree to better looking.


one_1 = onnx.helper.make_tensor('one_1', onnx.TensorProto.FLOAT, [1], [1])
ones = gb.op('ConstantOfShape', [n_rows], value=one_1)
row_idxs = gb.op('Squeeze', [gb.op('NonZero', [ones])])
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the comment of "the process is equivalent to get range"

@@ -155,6 +155,11 @@
'args': {'slices': (slice(None), slice(0, 1), slice(None, 2))},
'name': 'get_item_start_from_none'},

# select_item
Copy link
Member

Choose a reason for hiding this comment

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

Could you split this test from TestArrayOperators, because this function is for unary args.

class TestSelectItem(ONNXModelTest):

    def test_output(self):

        class Model(chainer.Chain):

            def forward(self, x, t):
                return F.select_item(x, t)

        model = Model()
        x = input_generator.increasing(3, 3)
        t = np.array([2, 1, 0], dtype=np.int32)

        self.expect(
            model, (x, t), expected_num_initializers=0,
            skip_opset_version=(7, 8))
  • set t as input, not to put unnecessary initializer
  • set skip opset version

@disktnk
Copy link
Member

disktnk commented Nov 18, 2019

@tkanmae Do you have special reason not to use GatherElements added from opset11? select_item could be repesented as GatherElements(Transpose(x), Unsqueeze(t)) (+ Squeeze) maybe.

@tkanmae
Copy link
Contributor Author

tkanmae commented Nov 18, 2019

@disktnk The reason I did not use GatherElements is that CoreML 3.0 does not support it. Thanks for the comments above. I will make changes as you suggested, hopefully in a few days.

@disktnk
Copy link
Member

disktnk commented Nov 19, 2019

CoreML 3.0 does not support it

I see. We'll make another PR to use GatherElement when opset_version >= 11, and this alternative logic is remained to support under opset_version < 11 environnment. thx

@tkanmae tkanmae force-pushed the support-set-item-in-onnx-chainer branch from c2345e1 to a3b6bf1 Compare November 19, 2019 09:55
@tkanmae tkanmae force-pushed the support-set-item-in-onnx-chainer branch from a3b6bf1 to 12376f5 Compare November 19, 2019 10:09
@tkanmae
Copy link
Contributor Author

tkanmae commented Nov 19, 2019

@disktnk

We'll make another PR to use GatherElement when opset_version >= 11, and this alternative logic is remained to support under opset_version < 11 environnment.

Got it.

I fixed the bug in testing select_item() conversion as you suggested, though I set skip_opset_version to [1, 2, 3, 4, 5, 6, 7, 8] because ConstantOfShape is available from opset 9. I confirmed that the test passes on my machine.

@disktnk disktnk self-requested a review November 19, 2019 10:14
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.

LTGM!

@disktnk disktnk added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Nov 19, 2019
@disktnk
Copy link
Member

disktnk commented Nov 19, 2019

Jenkins, test this please!

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 12376f5, target branch master) succeeded!

@disktnk
Copy link
Member

disktnk commented Nov 20, 2019

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

@disktnk disktnk merged commit e610a24 into chainer:master Nov 20, 2019
@kmaehashi kmaehashi added this to the v7.0.0 milestone Dec 5, 2019
@kmaehashi kmaehashi added the cat:enhancement Implementation that does not break interfaces. label Dec 5, 2019
@emcastillo emcastillo changed the title Add support of SelectItem in ONNX-Chainer Add support of SelectItem in ONNX-Chainer Dec 5, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 12376f5, target branch master) succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces. ONNX Related to ONNX st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants