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 shapeinference function #2296

Merged
merged 11 commits into from
Sep 12, 2019

Conversation

jignparm
Copy link
Contributor

Updated the shape inference function for CumSum.

@wschin
Copy link
Contributor

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
Contributor Author

Could you please add a test into this file?

Done.

Copy link
Contributor

@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
Contributor

wschin commented Sep 11, 2019

@ebarsoum, how does this PR look to you?

@wschin wschin added the operator Issues related to ONNX operators label Sep 11, 2019
Copy link
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
operator Issues related to ONNX operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants