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

Tree-LSTM implementation for ChainerX #7720

Merged
merged 12 commits into from
Aug 26, 2019
Merged

Tree-LSTM implementation for ChainerX #7720

merged 12 commits into from
Aug 26, 2019

Conversation

dido1998
Copy link
Contributor

@dido1998 dido1998 commented Jul 7, 2019 •
edited
Loading

This depends on #7881

@asi1024 asi1024 added the GSoC label Jul 25, 2019
@dido1998
Copy link
Contributor Author

Can this be reviewed?

@niboshi
Copy link
Member

niboshi commented Aug 20, 2019

Some parts in this PR is overlapping with #7783. I'll merge this PR after that.
Triggering CIs for testing anyway.

@niboshi
Copy link
Member

niboshi commented Aug 20, 2019

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit fa8a90c, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member

niboshi commented Aug 22, 2019

Jenkins, test this please

@chainer chainer deleted a comment from chainer-ci Aug 22, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 4a7fd65, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member

niboshi commented Aug 22, 2019

Please resolve conflict.

@niboshi niboshi added the cat:feature Implementation that introduces new interfaces. label Aug 22, 2019
@niboshi niboshi changed the title Tree lstm implementation for ChainerX Tree-LSTM implementation for ChainerX Aug 22, 2019
@niboshi
Copy link
Member

niboshi commented Aug 22, 2019

Thanks! Jenkins, test this please

There's a docs error in Travis CI. Please fix it.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 182db2f, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member

niboshi commented Aug 23, 2019

Additionally to the Travis error, Jenkins reports a clang-tidy error:

08:56:07 /repo/chainerx_cc/chainerx/routines/activation.cc:109:5: error: use range-based for loop instead [modernize-loop-convert,-warnings-as-errors]
08:56:07     for (size_t i = 0; i < fs.size(); i  ) {
08:56:07     ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
08:56:07         (const auto & f : fs)

niboshi
niboshi requested changes Aug 23, 2019
View reviewed changes
chainerx_cc/chainerx/routines/activation.cc Outdated
Array i_ = Sigmoid(i);
Array o_ = Sigmoid(o);
std::vector<Array> fs_s{};
for (const auto & f : fs) {
Copy link
Member

@niboshi niboshi Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think clang-format should complain about this space (auto &).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dido1998
Copy link
Contributor Author

travis-ci is showing exceeded time-limit error

@niboshi
Copy link
Member

niboshi commented Aug 23, 2019

Reran the test.
Jenkins test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 8c6abe6, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member

niboshi commented Aug 23, 2019

Jenkins test this please

@niboshi niboshi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Aug 23, 2019
@niboshi niboshi added this to the v7.0.0b4 milestone Aug 23, 2019
@chainer-ci
Copy link
Member

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

@mergify mergify bot merged commit cb2a353 into chainer:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niboshi niboshi niboshi approved these changes

Assignees

@niboshi niboshi

Labels
cat:feature Implementation that introduces new interfaces. GSoC st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Milestone
v7.0.0b4
Development

Successfully merging this pull request may close these issues.
4 participants
@dido1998 @niboshi @chainer-ci @asi1024