Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add chainerx.cumsum #7558

Merged
merged 31 commits into from
Jul 19, 2019
Merged

Add chainerx.cumsum #7558

merged 31 commits into from
Jul 19, 2019

Conversation

aksub99
Copy link
Contributor

@aksub99 aksub99 commented Jun 20, 2019 •
edited
Loading

Add chainerx.cumsum.

@asi1024 asi1024 self-assigned this Jun 20, 2019
@asi1024 asi1024 added cat:feature Implementation that introduces new interfaces. ChainerX Related to ChainerX. labels Jul 1, 2019
@aksub99
Copy link
Contributor Author

aksub99 commented Jul 4, 2019

I got rid of the chainerx._testing travis failure. The native implementation of cumsum still doesn't work as expected. Will make the fixes ASAP.

@aksub99
Copy link
Contributor Author

aksub99 commented Jul 4, 2019 •
edited
Loading

Fixed the implementation (Seems to work correctly when I tested locally). Only tests are remaining.
TODO: Fix tests and implement backward.

@aksub99 aksub99 marked this pull request as ready for review July 4, 2019 04:09
@aksub99
Copy link
Contributor Author

aksub99 commented Jul 5, 2019

The tests are failing for float16 and bool inputs.

@asi1024
Copy link
Member

asi1024 commented Jul 5, 2019

numpy.cumsum returns int64 ndarray when bool or integer ndarray has taken as an input. chainerx.cumsum should also follow this dtype rule.

@aksub99 aksub99 changed the title [WIP] Add chainerx.cumsum Add chainerx.cumsum Jul 5, 2019
@aksub99
Copy link
Contributor Author

aksub99 commented Jul 5, 2019

Have made the suggested changes.

@asi1024 asi1024 mentioned this pull request Jul 8, 2019
Closed
@aksub99
Copy link
Contributor Author

aksub99 commented Jul 8, 2019

@asi1024 I have added the backward implementation. The backward tests pass but the double backward tests fail. Could you have a look at the implementation please?
I suspect the problem might be with the usage of Cumsum itself in the backward implementation.

@aksub99
Copy link
Contributor Author

aksub99 commented Jul 10, 2019

Have rebased with master to include bug fix in Flip from #7727 . The tests pass now.

asi1024
asi1024 reviewed Jul 10, 2019
View reviewed changes
chainerx_cc/chainerx/routines/reduction.cc Outdated Show resolved Hide resolved
chainerx_cc/chainerx/routines/reduction.cc Outdated Show resolved Hide resolved
chainerx_cc/chainerx/routines/reduction.cc Outdated Show resolved Hide resolved
chainerx_cc/chainerx/native/native_device/reduction.cc Show resolved Hide resolved
chainerx_cc/chainerx/routines/reduction.cc Outdated
if (axis.has_value()) {
bctx.input_grad() = Flip(Cumsum(Flip(gout, axis_norm), axis_norm), axis_norm);
} else {
Array input_grad = Flip(Cumsum(Flip(gout, 0), 0), 0);
Copy link
Member

Choose a reason for hiding this comment

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

gout will be flipped twice during the double-backward. It can be reduced.

@asi1024 asi1024 added GSoC st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. labels Jul 12, 2019
@aksub99
Copy link
Contributor Author

aksub99 commented Jul 18, 2019

I have added a TODO to improve backward implementation.

@aksub99
Copy link
Contributor Author

aksub99 commented Jul 19, 2019

Is this ready to merge @asi1024 ? Can I go ahead with cumprod and nonzero ?

@asi1024 asi1024 removed the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Jul 19, 2019
@asi1024
Copy link
Member

asi1024 commented Jul 19, 2019

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 227778f, target branch master) succeeded!

@asi1024
Copy link
Member

asi1024 commented Jul 19, 2019

LGTM. Thank you for the PR!

@asi1024 asi1024 added this to the v7.0.0b3 milestone Jul 19, 2019
@asi1024 asi1024 merged commit 493a768 into chainer:master Jul 19, 2019
@asi1024 asi1024 mentioned this pull request Aug 30, 2019
Merged
@asi1024 asi1024 mentioned this pull request Sep 10, 2019
Merged
@take-cheeze take-cheeze mentioned this pull request Oct 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asi1024 asi1024 asi1024 left review comments

Assignees

@asi1024 asi1024

Labels
cat:feature Implementation that introduces new interfaces. ChainerX Related to ChainerX. GSoC
Projects
None yet
Milestone
v7.0.0b3
Development

Successfully merging this pull request may close these issues.
3 participants
@aksub99 @asi1024 @chainer-ci