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

[v1.7.x] Backport some numpy features + fixes #18648

Closed
wants to merge 44 commits into from
Closed

[v1.7.x] Backport some numpy features + fixes #18648

wants to merge 44 commits into from

Conversation

sxjscience
Copy link
Member

Description

(Brief description on what this PR is about)

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

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@mxnet-bot
Copy link

Hey @sxjscience , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [windows-gpu, centos-cpu, edge, windows-cpu, unix-gpu, miscellaneous, unix-cpu, website, clang, sanity, centos-gpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

Yiyan66 and others added 11 commits June 30, 2020 22:18
@Yiyan66 @sxjscience
flatnonzero (apache#17690)
b9c6a4a
@haojin2 @sxjscience
[Numpy] FFI for cumsum and add (apache#17747)
8b5cb5e 
* FFI cumsum

* Dispatch ufunc

* Add PythonArg

* Remove unused data type

* Seperate op_utils and utils
@Tommliu @sxjscience
[Numpy] FFI: Bincount, Percentile/Quantile, All/Any (apache#17717)
e8e320e 
* ffi_bincount percentile/quantile all/any

* new ffi
@hzfan @meta-project-ci @sxjscience
Add ffi benchmark (apache#17780)
b88092e 
Co-authored-by: Haozheng Fan <[email protected]>
@Alicia1529 @sxjscience
ffi wrappers for polyval, ediff1d, nan_to_num (apache#17832)
2a3dbda
@hzfan @sxjscience
Fix compiler warnings in new FFI (apache#17718)
8750350 
Introduced in apache#17510
@Alicia1529 @sxjscience
ffi invocation: expand_dims, tril, diff, broadcast_to (apache#17738)
8c90040
@hzfan @meta-project-ci @sxjscience
[Numpy] FFI for split and svd (apache#17816)
e406293 
* Support ADT as FFI return value

* Special operator= for NDArrayHandle

* SVD

* Support cython

* Clear

* Add split

* Refine

* Fix ci

* Fix typo

* Clear

* Resolve sanity issues

Co-authored-by: Haozheng Fan <[email protected]>
@Alicia1529 @sxjscience
add ffi for full_like, binary (apache#17811)
7ed6bf7
@sjtuWangDing @sxjscience
* impl - FFI for np_where_op (apache#17817)
a81d5f4 
* impl - FFI for np_may_share_memory

* impl - FFI benchmark

Co-authored-by: Ubuntu <[email protected]>
@Tommliu @sxjscience
ffi for roll/rot90 (apache#17861)
b816d43
JiangZhaoh and others added 3 commits June 30, 2020 23:40
@JiangZhaoh @sxjscience
[Numpy] allow mix integer dtypes for power/add/multiply (apache#17921)
34b4708 
* resolution

* fix sanity error

* remove func 'is_integer'
@sxjscience
fix true_divide (apache#18393)
aedf66b
@sjtuWangDing @sxjscience
* FFI for np.argmax and np.argmin (apache#17843)
f862579 
* impl - FFI for np_indices

* fix - use MXNetTypeWithBool2String

Co-authored-by: Ubuntu <[email protected]>
@sxjscience
Copy link
Member Author

sxjscience commented Jul 1, 2020 •
edited
Loading

@ciyongch I traced the commits that are before 743bbcb and these are the initial list of commits.

There are two remaining commits like:

Also, there is one commit that I'm not sure whether it should be backported or not:

haojin2 and others added 9 commits July 1, 2020 00:11
@haojin2 @sxjscience
add alias for np.__version__, np._NoValue and np.dtype (apache#17777)
f00c3b5
@Tommliu @sxjscience
[Numpy] FFI for diag/diagonal/diag_indices_from (apache#17789)
27d5008 
* ffi_diag/diagonal/diag_indices_from

* sanity && benchmark
@Tommliu @sxjscience
ffi_atleast_1/2/3d (apache#17897)
905eb27
@yijunc @sxjscience
add: numpy rollaxis (apache#17865)
acb535b
@sjtuWangDing @sxjscience
* impl - FFI for np einsum (apache#17869)
ce37bef 
* impl - FFI for np dstack

* impl - benchmark np_einsum np_dstack

* impl - FFI for np_unique

* impl - benchmark np_unique

Co-authored-by: Ubuntu <[email protected]>
@Yiyan66 @sxjscience
[numpy] add op random.f (apache#17586)
28dbfda 
* F

* f
@AntiZpvoh @sxjscience
[NumPy] add op random.laplace (apache#17316)
573830a 
* NumPy Laplace Distribution partly Frontend and Backend

Signed-off-by: AntiZpvoh <[email protected]>

* NumPy Laplace Distribution Backend style rectified

Signed-off-by: AntiZpvoh <[email protected]>

* NumPy Laplace Distribution Frontend modified

Signed-off-by: AntiZpvoh <[email protected]>

* Laplece op nightly test and normal op test correction

Signed-off-by: AntiZpvoh <[email protected]>

* NumPy Laplace Distribution unit test and code style

Signed-off-by: AntiZpvoh <[email protected]>

* Register uniform_n in CUDA

Signed-off-by: AntiZpvoh <[email protected]>

* Delete the registering of Laplace_n

Signed-off-by: AntiZpvoh <[email protected]>

* fix some alignment and indentation problems

Signed-off-by: AntiZpvoh <[email protected]>

* fix some sanity problems such as too long lines

* fix some sanity problems again

* laplace parmeters form change

* implement basic laplace function

* add frontend implement and ndarray loc case

* complete the frontend

* fix some sanity problems

* fix some sanity problems

* fix some typos

* fix some problems

* fix a typo

* add size==() condition handling

* fix some typos

* remove unused code

Co-authored-by: Ubuntu <[email protected]>
@sjtuWangDing @sxjscience
* impl - FFi for linalg op (apache#17795)
fd17146 
* fix - cpplint

* impl - benchmark ffi for ops

* rm - FFI for ops with param

* fix - makefile

* fix - not include unordered_map and use num_inputs

* ci - compiler error

* fix - change cholesky interface

Co-authored-by: Ubuntu <[email protected]>
@hzfan @sxjscience
[Numpy] FFI: split and svd apache#17816
5a9bf61
@sxjscience sxjscience changed the title [v1.7.x] [Backport]add zero grad for npi_unique (#18080) [v1.7.x] Backport some numpy features + fixes Jul 1, 2020
sjtuWangDing and others added 12 commits July 1, 2020 00:49
@sjtuWangDing @sxjscience
* impl - linalg matrix_rank for cpu and gpu implemented (apache#18020)
ff2dbab 
* fix - python interface

* impl - ffi for matrix_rank

* impl - ffi benchmark

Co-authored-by: Ubuntu <[email protected]>
@hanke580 @sxjscience
[Numpy] Kron operator (apache#17323)
20f70e4 
* [Numpy]Add kron

* Implement the forward of Kron op

* Implement the Backward of a

* Implement the Backward of b

* Fix 3rd party

* Fix cpp sanity

* Finish grad check

* address comments: fix test_np_op and reduce req to req[0]

* * Fix  ndim  = 0

* * Fix uninitialize bugs

* * Impl FFI
@Tommliu @sxjscience
[Numpy] OP_interp (apache#17793)
2f69252 
* interp

* fix_uninitialized_issue
@Yiyan66 @sxjscience
[numpy] add op median apache#17084
6fe903b
@hanke580 @sxjscience
[Numpy] Add op fmax, fmin, fmod (apache#17567)
3f195fb
@yijunc @sxjscience
add: numpy op tril_indices (apache#17904)
673d3b3
@vexilligera @haojin2 @sxjscience
[NumPy] Add NumPy support for triu (apache#17614)
5ba7a77 
* triu

* rebase

* fix ci

* merge

* triu new ffi

* cpplint

* cpplint

* ffi benchmark

* fix style

* merge

* fix conflict

Co-authored-by: Ubuntu <[email protected]>
Co-authored-by: Hao Jin <[email protected]>
@yijunc @sxjscience
fix mixed type backward (apache#18250)
9ec1c4b
@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
@Yiyan66 @sxjscience
[numpy] FFI binary bitwise ops (apache#17812)
2e57a6b 
* ffi_bitwise  binary

* retrigger ci
@AntiZpvoh @sxjscience
[Numpy] FFI: random.choice, take and clip (apache#17854)
1944c70 
* change the header file of np.random.choice

* add np_choice_op.cc file

* add including header file

* implement the basic function of random.choice

* try to use take op in backend

* try to use take op in backend

* add take invoking function

* fix some syntax problems

* fix some problems

* complete numpy.random.choice ffi

* first commit of ffi indexing_op.cc

* add random.choice ffi benchmark

* complete take ffi

* change the implementation of random.choice

* add take op benchmark

* complete clip op ffi and fix a problem

* add clip op benchmark

* fix some sanity problems

* add space before ( and fix reimport

* fix a typo

* remove dead code and remove new operator

Co-authored-by: Ubuntu <[email protected]>
@xidulu @sxjscience
fix np.clip scalar input case (apache#17788)
327f7ad
@sxjscience
Copy link
Member Author

@ciyongch I find that there are lots of numpy stuffs here. This is the initial attempt for backporting some commits. One issue is #17645, in which I'm not sure whether to port or not.

@sxjscience sxjscience mentioned this pull request Jul 1, 2020
Open
@ciyongch
Copy link
Contributor

ciyongch commented Jul 1, 2020

Thanks @sxjscience for the effort.
It seems that this backport introduce huge code changes which will be a big concern to the current stable code base especially after code freeze, then I'm more preferred NOT to include this at 1.7 release, what do you think? Copy @szha as well.

@sxjscience
Copy link
Member Author

sxjscience commented Jul 1, 2020 via email

@sxjscience
Copy link
Member Author

sxjscience commented Jul 1, 2020 via email

@ciyongch
Copy link
Contributor

ciyongch commented Jul 1, 2020

As numpy operator feature is in a quite active development in current master branch and mainly targeting on 2.0 release. It'll be fine if the backport patch is small and controllable at this time frame, but given the current one is introducing substantial modifications (+16,079 −1,160 code changes in 192 files), which is really a big concern whether this patch can help to make it solid enough while not introducing any other numpy issue. I agree to include existing fixes as much as possible, but this one is kind of complicated and need further confirmation. What do you think @szha @sandeep-krishnamurthy ?
Quote from @sandeep-krishnamurthy

+1 to mark this as experimental as it was not stable in 1.6 or advertised as will be stable in v1.7. We are mainly looking at numpy ops in 2.0.

Thanks.

@sandeep-krishnamurthy
Copy link
Contributor

First of all thank you very much @sxjscience for this tremendous effort in backporting PRs.

I am really concerned with addition of new experimental features in the last minute. 1.7 was stable with many required functionality for users and no known breaking new changes compared to v1.6.

I still believe all numpy related changes in these commits should go in a separate release and not block v1.7 as I believe this is not a stable feature set expected by users or advertised 2 months ago before code freeze as v1.7 feature.

@sxjscience
Copy link
Member Author

@sandeep-krishnamurthy @ciyongch I created this PR to track how many numpy commits would potentially go into v1.7.0 (not all of them should go into 1.7.0):

We may

  • Ignore all the FFI commits
  • Ignore commits that add new operators

However, we should add all the bug fixes

Apart from the following two commits

We will also need

@sandeep-krishnamurthy
Copy link
Contributor

@sandeep-krishnamurthy @ciyongch I created this PR to track how many numpy commits would potentially go into v1.7.0 (not all of them should go into 1.7.0):

We may

  • Ignore all the FFI commits
  • Ignore commits that add new operators

However, we should add all the bug fixes

Apart from the following two commits

We will also need

Thank you @sxjscience - Can you please help with back port of these 4 bug fix PRs?

@ciyongch
Copy link
Contributor

ciyongch commented Jul 2, 2020

Thanks a lot @sxjscience for the great help to filter the necessary PRs targeting at 1.7 release and @sandeep-krishnamurthy for the valuable comments :)
When you're deciding to backport these PRs, are there any additional cases to make sure they're working expected at your side?

@sandeep-krishnamurthy
Copy link
Contributor

@sxjscience when will it be possible to have a Backport PRs? @ciyongch what is the timeline you had planned for rc0?

@ciyongch
Copy link
Contributor

ciyongch commented Jul 2, 2020

Hi @sandeep-krishnamurthy , actually I was planning to drop rc0 before numpy related issues were raised.
@sxjscience has already submitted partial PRs listed as above in #18653. The remaining PRs are (#18250 and #18523), it would be great if the rest of PRs can be backported within 48h. @sxjscience can you please help to give an estimate for the remaining effort, or we might need to cut off some fixes/PRs if it's uncertain or taking too much time. What do you think? Thanks!

@sxjscience
Copy link
Member Author

@ciyongch I'm going to test v1.7 after this backporting PR (#18653) is merged. WOuld you merge that one?

@sxjscience
Copy link
Member Author

Close this PR now

@sxjscience sxjscience closed this Jul 2, 2020
@ciyongch
Copy link
Contributor

ciyongch commented Jul 2, 2020

Sure, that one should be fine, how about the rest of two PRs? May I know how much effort and time you'll need to backport them? Thanks!

@ciyongch
Copy link
Contributor

ciyongch commented Jul 2, 2020

@TaoLv helped to merge that PR, please help to check if the current code base works expected or not :)

@yijunc yijunc mentioned this pull request Jul 2, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.