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

[Numpy] Add ffi for np.sum, np.std, np.var and np.average #17866

Merged
merged 3 commits into from
Apr 16, 2020
Merged

[Numpy] Add ffi for np.sum, np.std, np.var and np.average #17866

merged 3 commits into from
Apr 16, 2020

Conversation

haojin2
Copy link
Contributor

@haojin2 haojin2 commented Mar 18, 2020 •
edited
Loading

Description

As title. together with some minor style fixes.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • FFI for sum, std, var
  • Change to outer's wrapper to avoid extra copying
  • Performance benchmark

Comments

Performance comparisons (in microseconds):

Operator Official NumPy Legacy ctypes New ctypes New Cython
outer 2.89125 85.96126 100.95388 47.191
sum 3.41371 30.65492 18.02886 5.97355
std 16.09432 51.83489 22.31538 9.23666
var 14.70022 49.0423 22.10497 9.27033
average 24.87307 53.8507 28.55034 12.93129

@haojin2 haojin2 added the Numpy label Mar 18, 2020
@haojin2 haojin2 requested a review from reminisce March 18, 2020 06:45
@haojin2 haojin2 requested a review from szha as a code owner March 18, 2020 06:45
@haojin2 haojin2 self-assigned this Mar 18, 2020
hzfan
hzfan approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@hzfan hzfan left a comment

Choose a reason for hiding this comment

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

LGTM, with some nits that can be optionally resolved.

python/mxnet/ndarray/numpy/_op.py Outdated Show resolved Hide resolved
src/api/operator/numpy/np_broadcast_reduce_op_value.cc Show resolved Hide resolved
@haojin2 haojin2 force-pushed the add_ffi branch 3 times, most recently from 11045b6 to de6c300 Compare March 25, 2020 05:38
@haojin2
Copy link
Contributor Author

haojin2 commented Mar 25, 2020

@mxnet-bot run ci [windows-gpu]

@mxnet-bot
Copy link

Hey @haojin2
@mxnet-bot can only be invoked on a PR.

@haojin2
Copy link
Contributor Author

haojin2 commented Mar 25, 2020

@mxnet-bot run ci [windows-gpu]

@mxnet-bot
Copy link

Hey @haojin2
@mxnet-bot can only be invoked on a PR.

@haojin2 haojin2 force-pushed the add_ffi branch 2 times, most recently from 4172c30 to 2607ace Compare March 27, 2020 07:49
@haojin2 haojin2 changed the title [Numpy] Add ffi for np.sum, np.var and np.std [Numpy] Add ffi for np.sum, np.std, np.var and np.average Mar 27, 2020
hzfan
hzfan reviewed Mar 30, 2020
View reviewed changes
Copy link
Contributor

@hzfan hzfan left a comment

Choose a reason for hiding this comment

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

average LGTM

hzfan
hzfan reviewed Apr 1, 2020
View reviewed changes
include/mxnet/runtime/ffi_helper.h Outdated Show resolved Hide resolved
python/mxnet/ndarray/numpy/_op.py Outdated Show resolved Hide resolved
src/api/operator/numpy/np_histogram_op.cc Outdated Show resolved Hide resolved
python/mxnet/ndarray/numpy/_op.py Outdated Show resolved Hide resolved
@haojin2 haojin2 force-pushed the add_ffi branch 2 times, most recently from cd84a1a to 2c6f3e2 Compare April 2, 2020 05:58
@haojin2 haojin2 force-pushed the add_ffi branch 3 times, most recently from 76d2a32 to 61e62c2 Compare April 10, 2020 05:55
@hzfan hzfan mentioned this pull request Apr 16, 2020
Merged
7 tasks
@haojin2 haojin2 merged commit 7bef85e into apache:master Apr 16, 2020
@haojin2 haojin2 deleted the add_ffi branch April 16, 2020 19:32
leezu added a commit to leezu/mxnet that referenced this pull request Apr 23, 2020
@leezu
Revert apache#17866
7493291 
This reverts commit 7bef85e due to
apache#18123.
sxjscience pushed a commit to sxjscience/mxnet that referenced this pull request Jul 1, 2020
@haojin2 @sxjscience
[Numpy] Add ffi for np.sum, np.std, np.var, np.average and np.histogr…
05551d0 
…am (apache#17866)

* add ffi for sum, var and std

* add ffi wrapper for np.average

* add ffi wrapper for np.histogram
AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this pull request Jul 6, 2020
@haojin2 @AntiZpvoh
[Numpy] Add ffi for np.sum, np.std, np.var, np.average and np.histogr…
fc2c20d 
…am (apache#17866)

* add ffi for sum, var and std

* add ffi wrapper for np.average

* add ffi wrapper for np.histogram
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hzfan hzfan hzfan approved these changes

@reminisce reminisce Awaiting requested review from reminisce

@szha szha Awaiting requested review from szha szha is a code owner

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

Assignees

@haojin2 haojin2

Labels
Numpy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haojin2 @mxnet-bot @hzfan