Skip to content

Fix shapeinference function#2296

Merged
wschin merged 11 commits intoonnx:masterfrom
jignparm:jignparm/fix_cumsum_shape_inference
Sep 12, 2019
Merged

Fix shapeinference function#2296
wschin merged 11 commits intoonnx:masterfrom
jignparm:jignparm/fix_cumsum_shape_inference

Conversation

@jignparm
Copy link
Copy Markdown
Contributor

Updated the shape inference function for CumSum.

@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 10, 2019

Could you please add a test into this file?

@prasanthpul prasanthpul added this to the 1.6 milestone Sep 10, 2019
@jignparm jignparm requested a review from a team as a code owner September 10, 2019 22:32
@jignparm
Copy link
Copy Markdown
Contributor Author

Could you please add a test into this file?

Done.

Copy link
Copy Markdown
Collaborator

@wschin wschin left a comment

Choose a reason for hiding this comment

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

Thanks!

@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 11, 2019

@ebarsoum, how does this PR look to you?

@wschin wschin added the topic: operator Issues related to ONNX operators label Sep 11, 2019
Copy link
Copy Markdown
Contributor

@spandantiwari spandantiwari left a comment

Choose a reason for hiding this comment

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

LGTM.

Aside - this is outside the scope of this change, but it may be useful to consider updating the cumsum op to allow specification of the output type from the user. For this kind of aggregation op it is quite possible that we run into overflow issues. Allowing the specification for output type allows the converter to choose a larger type for internal aggregation the cumulative sum and output.

@wschin wschin merged commit 95252c2 into onnx:master Sep 12, 2019
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
* Fix shapeinference function

* Added shapeinference test for cumsum

* update inference test

* fix test

* minor fix -- shape (1) should be (1,)

* Add whitespace after comma to fix flake warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: operator Issues related to ONNX operators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants