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

Fix ElemwiseSum for more than 4 inputs #17995

Merged
merged 2 commits into from
Apr 10, 2020
Merged

Fix ElemwiseSum for more than 4 inputs #17995

merged 2 commits into from
Apr 10, 2020

Conversation

ptrendx
Copy link
Member

@ptrendx ptrendx commented Apr 7, 2020

Description

Fixes #17989

It was caused by a bug in ElemwiseSum which for more than 4 inputs and kAddTo req was counting gradients multiple times.

@sxjscience @zhreshold Please test if this fixes the issues you saw.

@mxnet-bot
Copy link

Hey @ptrendx , 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-cpu, miscellaneous, unix-gpu, edge, windows-gpu, website, sanity, centos-cpu, clang, centos-gpu, unix-cpu]

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.

@leezu
Copy link
Contributor

leezu commented Apr 7, 2020

Should we add a test case in the style of #17989 (comment) to ensure no such bug makes it into the codebase again?

@sxjscience
Copy link
Member

@ptrendx I can confirm that this fixes the issue.

@zhreshold
Copy link
Member

confirmed it can fix #16708 (comment) as well

@szha
Copy link
Member

szha commented Apr 8, 2020

@ptrendx thanks for the fix. Let's add a test for this.

@ptrendx ptrendx mentioned this pull request Apr 9, 2020
Closed
@szha
Copy link
Member

szha commented Apr 10, 2020

@mxnet-bot run ci [centos-gpu, unix-gpu, windows-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu, centos-gpu, windows-gpu]

@ptrendx ptrendx added the v1.x Targeting v1.x branch label Apr 10, 2020
@ptrendx
Copy link
Member Author

ptrendx commented Apr 10, 2020

Added v1.x label even though this PR is targeted for master in order to track that we want it included in 1.7 release @ciyongch FYI.

@sxjscience sxjscience merged commit 07b8d7a into apache:master Apr 10, 2020
@zhreshold zhreshold mentioned this pull request Apr 10, 2020
Closed
@ciyongch
Copy link
Contributor

@ptrendx can you help to backport this PR to v1.x branch? Then I'll rebase v1.7.x to v1.x accordingly. I'm adding this to 1.7.0 roadmap #16864.

ptrendx added a commit to ptrendx/mxnet that referenced this pull request Apr 13, 2020
@ptrendx
Fix ElemwiseSum for more than 4 inputs (apache#17995)
2d27df3 
* Fix ElemwiseSum for more than 4 inputs

* Added test
@ptrendx ptrendx mentioned this pull request Apr 13, 2020
Merged
ptrendx added a commit to ptrendx/mxnet that referenced this pull request Apr 14, 2020
@ptrendx
Fix ElemwiseSum for more than 4 inputs (apache#17995)
64c823f 
* Fix ElemwiseSum for more than 4 inputs

* Added test
ptrendx added a commit that referenced this pull request Apr 18, 2020
@ptrendx
Cherry-pick of #17995 and #17937 to 1.x branch (#18041)
814530d 
* Fix ElemwiseSum for more than 4 inputs (#17995)

* Fix ElemwiseSum for more than 4 inputs

* Added test

* Fix for handling negative indices in the fusion of slice (#17937)

* Fix for handling of negative axis, begin and end in fusion of slice ops

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

@sxjscience sxjscience sxjscience approved these changes

Assignees
No one assigned
Labels
v1.x Targeting v1.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Autograd] Very serious bug of grad_req='add'
7 participants
@ptrendx @mxnet-bot @leezu @sxjscience @zhreshold @szha @ciyongch