Fix shape inference for Split with split attribute #2328

shinh · 2019-09-18T00:27:38Z

This fixes #1735

CLAassistant · 2019-09-18T00:27:48Z

All committers have signed the CLA.

hariharans29 · 2019-09-18T23:47:24Z

onnx/defs/tensor/defs.cc

+            axis += rank;
+          }
+          const auto& splitDim = shape.dim(axis);
+          if (!splitDim.has_dim_value()) {


we can still do rank inference as the ranks of the outputs will be the same as the rank of the input before returning.

hariharans29 · 2019-09-18T23:48:45Z

onnx/defs/tensor/defs.cc

            }
-            const auto& splitDim = shape.dim(axis);
-            if (!splitDim.has_dim_value()) {
+            if (totalDim != splitDimValue) {
              return;


should this be an error ?

hariharans29 · 2019-09-18T23:55:51Z

onnx/defs/tensor/defs.cc

              return;
            }
-            int splitDimValue = static_cast<int>(splitDim.dim_value());
+          } else {
            int chunkSize =


Per spec, it says if 'split' is not provided, split it into equal sized parts, so should the case where splitDimValue % ctx.getNumOutputs() != 0 be an error condition ?

Otherwise, we might have to arbitrarily deal with it like the logic in 444-448. The logic is not prescribed per spec. So I think this should be an error condition as well. To me there are 3 possible error conditions in this op-

When 'split' is present and its count does not match number of outputs

When 'split' is present and its sum does not match input shape's corresponding value in the 'axis' dimension

When 'split' is not present and input shape's corresponding value in the 'axis' dimension cannot be split evenly to the number of outputs

shinh · 2019-09-19T01:18:55Z

Thanks for your review! I think I handled all of your comments. Please take a look again?

hariharans29 · 2019-09-19T20:41:01Z

onnx/test/shape_inference_test.py

+        self._assert_inferred(graph, [make_tensor_value_info('y', TensorProto.FLOAT, (2, None, 'b')),  # type: ignore
+                                      make_tensor_value_info('z', TensorProto.FLOAT, (2, None, 'b'))])  # type: ignore
+
+    def test_split_fail_with_invalid_split_attribute(self):  # type: () -> None


Nit: I don't think we explicitly add tests for failure scenarios. Seems like these 2 may not be needed.

hariharans29 · 2019-09-19T20:42:49Z

onnx/test/shape_inference_test.py

@@ -1097,6 +1097,36 @@ def test_split_negative_axis(self):  # type: () -> None
        self._assert_inferred(graph, [make_tensor_value_info('y', TensorProto.FLOAT, (2, 2)),
                                      make_tensor_value_info('z', TensorProto.FLOAT, (2, 2))])

+    def test_split_with_split_attribute(self):  # type: () -> None


can you please add 2 more tests -

One with split attribute is missing and the default 'equal' split occurs for each output

One with (valid) negative axis

Isn't the existing test case, test_split_negative_axis enough to cover these two cases? Do you want me to split it into two tests?

You are right. :) I did not notice the other test.

hariharans29 · 2019-09-19T20:44:50Z

onnx/defs/tensor/defs.cc

+            }
+            int chunkSize = splitDimValue / numOutputs;
+            for (int i = 0; i < static_cast<int>(ctx.getNumOutputs()); i++) {
+              split.push_back(chunkSize);


nit 1: It seems a little ugly to store the same value 'ChunkSize' in the vector for each output. it would be great if the single value 'chunkSize' can just be handled below when you actually assign the dim value.

nit 2: 'chunkSize' doesn't seem c++ style. You can consider using 'chunk_size'

As for 2, fixed.

As for 1, let me double-check if you like the change, since I feel current code is better. If I follow your suggestion, I think we'll have two copies of the for loop which assigns dim_value like

if (getRepeatedAttribute(ctx, "split", split)) { ... for (size_t i = 0; i < ctx.getNumOutputs(); i++) { *ctx.getOutputType(i)->mutable_tensor_type()->mutable_shape() = shape; ctx.getOutputType(i) ->mutable_tensor_type() ->mutable_shape() ->mutable_dim(axis) ->set_dim_value(split[i]); } } else { ... int chunk_size = split_dim_value / num_outputs; for (size_t i = 0; i < ctx.getNumOutputs(); i++) { *ctx.getOutputType(i)->mutable_tensor_type()->mutable_shape() = shape; ctx.getOutputType(i) ->mutable_tensor_type() ->mutable_shape() ->mutable_dim(axis) ->set_dim_value(chunk_size); } }

Is this what you want?

That's fine. We can just keep it the way it is for now.

hariharans29 · 2019-09-19T20:45:32Z

Thanks for your review! I think I handled all of your comments. Please take a look again?

Just some minor comments to be addressed. Looks good otherwise. :)

shinh · 2019-09-20T02:04:36Z

Thanks for your review! Let me check a couple of things before I actually change them.

This fixes onnx#1735

shinh · 2019-09-24T09:52:02Z

The test was randomly failing so I pushed it several times to trigger the rebuild.

shinh · 2019-09-27T02:25:30Z

@hariharans29 Ping?

hariharans29

LGTM. @wschin ?

shinh · 2019-10-04T03:28:23Z

@wschin: Friendly reminder :)

hariharans29 · 2019-10-04T04:29:07Z

@gramalingam @ebarsoum @houseroad - pinging more people for reviews

@shinh - Don't worry. This PR has been tagged for release 1.7. 1.6 just got done last week. So it will be picked up when the PRs start getting merged for the next release (maybe sooner).

shinh · 2019-10-04T07:14:10Z

@hariharans29: Got it, thanks! I'm worried another person makes a similar patch and there'll be duplicated effort. It happened once in #1855 (and you were the author of #2041 :).

hariharans29 · 2019-10-04T07:32:30Z

@shinh - I see. Sorry, I should have checked if there was already a PR for Expand shape inference. Someone on my team needed it urgently, and I just wrote it without checking for any existing PRs.

onnx#2328 did not fix the shape inference of the old Split.

* Fix shape inference for Split with split attribute This fixes onnx#1735 * Set rank even when split dimension is unknown * Let shape inference fail for invalid split * Fail shape inference for inequal division * Ignore Python type error in the unittest * Remove tests for failure scenarios * Stop using camel case for local variables Co-authored-by: Ke Zhang <kezhan@microsoft.com>

This should be same as onnx#2328

This should be same as onnx#2328 Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com>

* Fix shape inference of Split-2 with split attr This should be same as #2328 Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> * Allow negative axis even in Split-2 Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> * Update onnx/defs/tensor/old.cc Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com>

shinh requested review from a team as code owners September 18, 2019 00:27

hariharans29 reviewed Sep 18, 2019

View reviewed changes

hariharans29 reviewed Sep 19, 2019

View reviewed changes

shinh force-pushed the fix-split-shape-infer branch from 7efbc6b to f4de1e2 Compare September 24, 2019 06:20

shinh added 6 commits September 24, 2019 15:25

Fix shape inference for Split with split attribute

883c0e8

This fixes onnx#1735

Set rank even when split dimension is unknown

3bdfe5d

Let shape inference fail for invalid split

5ddbd68

Fail shape inference for inequal division

e083a9f

Ignore Python type error in the unittest

031020b

Remove tests for failure scenarios

c58dd5a

shinh force-pushed the fix-split-shape-infer branch from f4de1e2 to e52f821 Compare September 24, 2019 06:25

Stop using camel case for local variables

fe79aeb

shinh force-pushed the fix-split-shape-infer branch from e52f821 to fe79aeb Compare September 24, 2019 08:08

wschin added this to the 1.7 milestone Sep 25, 2019

wschin added the operator Issues related to ONNX operators label Sep 25, 2019

hariharans29 approved these changes Sep 27, 2019

View reviewed changes

shinh mentioned this pull request Oct 17, 2019

Fixes to runtime/compiler configurations pfnet-research/chainer-compiler#768

Merged

2 tasks

postrational approved these changes Nov 13, 2019

View reviewed changes

shinh and others added 3 commits November 15, 2019 16:14

Merge branch 'master' into fix-split-shape-infer

6fa9555

Merge branch 'master' into fix-split-shape-infer

e2e1e6a

Merge branch 'master' into fix-split-shape-infer

85c988d

linkerzhang approved these changes Dec 3, 2019

View reviewed changes

shinh added 2 commits December 5, 2019 22:38

Merge branch 'master' into fix-split-shape-infer

17036de

Merge branch 'master' into fix-split-shape-infer

48087b2

linkerzhang merged commit dfa4384 into onnx:master Jan 14, 2020

shinh added a commit to shinh/onnx that referenced this pull request Jan 15, 2020

Fix shape inference of Split-2

3723831

onnx#2328 did not fix the shape inference of the old Split.

shinh mentioned this pull request Jan 15, 2020

Fix shape inference of Split-2 #2549

Merged

shinh added a commit to shinh/onnx that referenced this pull request Feb 27, 2020

Fix shape inference of Split-2

989953b

onnx#2328 did not fix the shape inference of the old Split.

shinh added a commit to shinh/onnx that referenced this pull request Dec 29, 2020

Fix shape inference of Split-2 with split attr

5a151c9

This should be same as onnx#2328

shinh added a commit to shinh/onnx that referenced this pull request Dec 29, 2020

Fix shape inference of Split-2 with split attr

6d9fe28

This should be same as onnx#2328 Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shape inference for Split with split attribute #2328

Fix shape inference for Split with split attribute #2328

shinh commented Sep 18, 2019

CLAassistant commented Sep 18, 2019 •

edited

Loading

hariharans29 Sep 18, 2019

shinh Sep 19, 2019

hariharans29 Sep 18, 2019

shinh Sep 19, 2019

hariharans29 Sep 18, 2019

hariharans29 Sep 19, 2019

shinh Sep 19, 2019

shinh commented Sep 19, 2019

hariharans29 Sep 19, 2019

shinh Sep 20, 2019

hariharans29 Sep 19, 2019

shinh Sep 20, 2019

hariharans29 Sep 27, 2019

hariharans29 Sep 19, 2019

shinh Sep 20, 2019

hariharans29 Sep 27, 2019

hariharans29 commented Sep 19, 2019

shinh commented Sep 20, 2019

shinh commented Sep 24, 2019

shinh commented Sep 27, 2019

hariharans29 left a comment

shinh commented Oct 4, 2019

hariharans29 commented Oct 4, 2019 •

edited

Loading

shinh commented Oct 4, 2019

hariharans29 commented Oct 4, 2019

Fix shape inference for Split with split attribute #2328

Fix shape inference for Split with split attribute #2328

Conversation

shinh commented Sep 18, 2019

CLAassistant commented Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinh commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariharans29 commented Sep 19, 2019

shinh commented Sep 20, 2019

shinh commented Sep 24, 2019

shinh commented Sep 27, 2019

hariharans29 left a comment

Choose a reason for hiding this comment

shinh commented Oct 4, 2019

hariharans29 commented Oct 4, 2019 • edited Loading

shinh commented Oct 4, 2019

hariharans29 commented Oct 4, 2019

CLAassistant commented Sep 18, 2019 •

edited

Loading

hariharans29 commented Oct 4, 2019 •

edited

Loading