Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[Numpy] cumprod #15855

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Numpy] cumprod #15855

wants to merge 1 commit into from

Conversation

hzfan
Copy link
Contributor

@hzfan hzfan commented Aug 12, 2019 •
edited

Description

Use tvm to implement numpy compatible cumprod

Changes

  • Add cumprod forward/backward with tvm
  • Add tests and docs

Comments

@hzfan hzfan requested a review from szha as a code owner August 12, 2019 04:38
@haojin2 haojin2 self-assigned this Aug 12, 2019
@haojin2 haojin2 added the Numpy label Aug 12, 2019
@haojin2 haojin2 added this to In progress in numpy via automation Aug 12, 2019
@haojin2 haojin2 requested review from yzhliu and removed request for szha August 12, 2019 06:25
yzhliu
yzhliu reviewed Aug 13, 2019
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

shall we put it into another namespace? it seems not to be "ufunc" in terms of numpy definition: https://docs.scipy.org/doc/numpy/reference/ufuncs.html

contrib/tvmop/basic/ufunc.py Outdated Show resolved Hide resolved
contrib/tvmop/basic/ufunc.py Outdated Show resolved Hide resolved
@hzfan
Copy link
Contributor Author

hzfan commented Aug 13, 2019

I created 2 new files: cumprod.py and cumprod.cc. If there are better namespaces, plz let me know.

@reminisce
Copy link
Contributor

Regarding the namespace choice in the frontend (python), how about we just follow what NumPy does? For example, cumprod is in numpy/core/fromnumeric.py. We can create tvmop/core/fromnumeric.py and put cumprod in it. This can serve as an easy and clear guideline for others to follow in the future.

@hzfan
Copy link
Contributor Author

hzfan commented Aug 14, 2019

Regarding the namespace choice in the frontend (python), how about we just follow what NumPy does? For example, cumprod is in numpy/core/fromnumeric.py. We can create tvmop/core/fromnumeric.py and put cumprod in it. This can serve as an easy and clear guideline for others to follow in the future.

Great! I created two files:

  • contrib/tvmop/core/fromnumeric.py
  • src/operator/contrib/tvmop/core/fromnumeric.cc

@reminisce reminisce mentioned this pull request Aug 23, 2019
Closed
7 tasks
haojin2
haojin2 reviewed Aug 23, 2019
View reviewed changes
contrib/tvmop/core/fromnumeric.py Outdated
import topi
from .. import defop, AllTypes, AllTypesButHalf

def kernel_cumprod(X, ishape, dtype, ndim, axis):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Maybe we can make this a common scan kernel, so that in the future similar scan operators could re-use this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good. I will do it. What about moving the generalized kernel to contrib/tvmop/utils.py?

@reminisce reminisce mentioned this pull request Aug 26, 2019
Open
yzhliu
yzhliu approved these changes Sep 16, 2019
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

LGTM, please rebase.

numpy automation moved this from In progress to Reviewer approved Sep 16, 2019
@hzfan
Copy link
Contributor Author

hzfan commented Sep 16, 2019

Have Rebased. And a minor change: move the dispatch of req to func set_req in src/operator/tvmop/utils.h so it can be reused.

@hzfan
Copy link
Contributor Author

hzfan commented Sep 16, 2019

Another minor modification: Rename src/operator/contrib/tvmop/core/fromnumeric.cc as src/operator/numpy/np_cumprod.cc (following the naming convention of cumsum.cc)

numpy automation moved this from Reviewer approved to Needs review Sep 17, 2019
marcoabreu
marcoabreu suggested changes Sep 17, 2019
View reviewed changes
ci/docker/runtime_functions.sh Outdated Show resolved Hide resolved
@yzhliu
Copy link
Member

yzhliu commented Sep 18, 2019

@hzfan Could you disable fp16 on cuda for now? We will fix it later.

@hzfan hzfan force-pushed the cumprod_pr branch 7 times, most recently from 0ddab9f to 6122559 Compare October 2, 2019 12:46
cumprod
3761cc0 
generalize accumulate kernel

define set_req as a util func

update tvm version

rename fromnumeric.cc as np_cumprod.cc

remove additional command

update tvm
@szha
Copy link
Member

szha commented Jul 27, 2020

@hzfan @yzhliu what's the status for this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reminisce reminisce reminisce left review comments

@haojin2 haojin2 haojin2 left review comments

@marcoabreu marcoabreu marcoabreu requested changes

@hgt312 hgt312 hgt312 left review comments

@yzhliu yzhliu yzhliu approved these changes

Assignees

@haojin2 haojin2

Labels
Numpy
Projects
No open projects
numpy
  
Needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@hzfan @reminisce @yzhliu @szha @haojin2 @marcoabreu @hgt312