[Topi, Relay] Add cumprod #7722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewZhaoLuo
changed the title
AndrewZhaoLuo
changed the title
|
You've got a merge issue, need to rebase. |
AndrewZhaoLuo
force-pushed
the
andrewzhaoluo-add-cumprod
branch
4 times, most recently
from
da75c66
to
4ead969
Compare
|
@masahi PTAL |
masahi
reviewed
Mar 24, 2021
There was a problem hiding this comment.
Looks nice. I suggest the following changes
- combineop -> scanop
- merge topi/cumsum.py and topi/cumprod.py into topi/scan.py (like the cuda version)
- merge test_topi_cumsum.py and test_topi_cumprod.py to remove duplication. Something like what you have in
relay/test_op_level3.py
AndrewZhaoLuo
force-pushed
the
andrewzhaoluo-add-cumprod
branch
from
a37657d
to
23d4325
Compare
|
@masahi PTAL. I've implemented all your suggested changes. |
added 16 commits
March 24, 2021 11:52
add cumprod and tests reinstate tests rethink
typos fix sum -> prod more typos more typo fixes more typos add doc strings
make exclusive a bool up and down stack fix x fix bool err it is a bool now fix fix thing formatting to pass linter lint python cumprod pylint fix attribute fix ordering add exclusivity tests for end to end fix things cuda identity_value
simplify construction clang-format more tests undo simpler construction due to function passing stuff fix docs more exclusive doc changes more fixins"
AndrewZhaoLuo
force-pushed
the
andrewzhaoluo-add-cumprod
branch
from
4137c15
to
b9fb779
Compare
AndrewZhaoLuo
force-pushed
the
andrewzhaoluo-add-cumprod
branch
from
e04e03d
to
dbea713
Compare
mbrookhart
reviewed
Mar 24, 2021
Comment on lines
+650
to
+654
| exclusive : bool, optional | ||
| If True, will return exclusive product in which the first element is not | ||
| included. In other terms, if True, the j-th output element would be | ||
| the product of the first (j-1) elements. Otherwise, it would be the product of | ||
| the first j elements. |
There was a problem hiding this comment.
I don't think numpy has an exclusive option? Do we need this? https://numpy.org/doc/stable/reference/generated/numpy.cumprod.html
There was a problem hiding this comment.
I'll defer to @masahi on this. I do agree that it adds a lot of complexity for not much gain.
For what it's worth, thrust has inclusive and exclusive scan operations which is what may have motivated the current interface.
There was a problem hiding this comment.
Hehehe, I tagged the wrong line. I know why cuda scan needs to be inclusive or exclusive (it's for other ops), but I'm not sure why cumprod needs the option?
There was a problem hiding this comment.
Yes, ONNX cumsum has an exclusive option. There is a good use case for exclusive cumsum but probably not for cumprod. We can always do inclusive scan for cumprod.
There was a problem hiding this comment.
Well we might as well keep cumprod's exclusive option since it has very little implementation overhead. The only thing that is there to support it is the identity_value fields which are pretty simple. I say we might as well keep it at this point.
masahi
reviewed
Mar 24, 2021
mbrookhart
approved these changes
Mar 24, 2021
jwfromm
approved these changes
Mar 24, 2021
|
thanks @AndrewZhaoLuo @mbrookhart @jwfromm |
trevor-m
pushed a commit
to trevor-m/tvm
that referenced
this pull request
May 6, 2021
* make cumbinop, refactor cumsum, add cumprod * cumsum exclusive test * Add cumprod + flesh out cumsum tests add cumprod and tests reinstate tests rethink * add rudimentary scan implementation * add attributes of cumprod node * add cumprod strategy * add cuda strategy * python relay node construction * change attrs to be reusuable * add cumprod nodes * complete tests * Fix some typos about sum --> prod typos fix sum -> prod more typos more typo fixes more typos add doc strings * Use Bool instead of int to represent exclusive make exclusive a bool up and down stack fix x fix bool err it is a bool now fix fix thing formatting to pass linter lint python cumprod pylint fix attribute fix ordering add exclusivity tests for end to end fix things cuda identity_value * Overall improve formatting, add doc message corrections simplify construction clang-format more tests undo simpler construction due to function passing stuff fix docs more exclusive doc changes more fixins" * merge cumsum and cumprod to scan, merge tests fix stuff * remove other mentions of cumbinop -> scanop * lint formatting Co-authored-by: Andrew Zhao Luo <andrewzhaoluo@Andrews-MacBook-Pro.local>
trevor-m
pushed a commit
to neo-ai/tvm
that referenced
this pull request
May 11, 2021
* make cumbinop, refactor cumsum, add cumprod * cumsum exclusive test * Add cumprod + flesh out cumsum tests add cumprod and tests reinstate tests rethink * add rudimentary scan implementation * add attributes of cumprod node * add cumprod strategy * add cuda strategy * python relay node construction * change attrs to be reusuable * add cumprod nodes * complete tests * Fix some typos about sum --> prod typos fix sum -> prod more typos more typo fixes more typos add doc strings * Use Bool instead of int to represent exclusive make exclusive a bool up and down stack fix x fix bool err it is a bool now fix fix thing formatting to pass linter lint python cumprod pylint fix attribute fix ordering add exclusivity tests for end to end fix things cuda identity_value * Overall improve formatting, add doc message corrections simplify construction clang-format more tests undo simpler construction due to function passing stuff fix docs more exclusive doc changes more fixins" * merge cumsum and cumprod to scan, merge tests fix stuff * remove other mentions of cumbinop -> scanop * lint formatting Co-authored-by: Andrew Zhao Luo <andrewzhaoluo@Andrews-MacBook-Pro.local>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extends PR #7334 to also have a numpy style cumprod operation.
Creates the notion of a cumulative binary operation. This does what it sounds and assumes the binary operation is associative and commutative. By passing in an identity value we can also extend the notion to exclusive cumulative sums/products. Examples of other operations which would fit this bill include binary xor as well as binary and.
@masahi @mbrookhart please select people who would be suitable to review.