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.

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

[chainerx] Implement Relu in c++ #6731

Merged
merged 7 commits into from Apr 12, 2019
Merged

Conversation

dido1998
Copy link
Contributor

@dido1998 dido1998 commented Apr 2, 2019

ref:

# TODO(imanishi): The function should also be available to C++ users

@beam2d beam2d added the ChainerX Related to ChainerX. label Apr 3, 2019
Copy link
Member

@asi1024 asi1024 left a comment

Choose a reason for hiding this comment

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

Could you fix for the C++/Python static checks?

Copy link
Member

@asi1024 asi1024 left a comment

Choose a reason for hiding this comment

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

LGTM except for the comment.

chainerx_cc/chainerx/routines/math.cc Outdated Show resolved Hide resolved


def relu(x):
"""Rectified Linear Unit function.
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this comment to chainerx/_docs/routines.py?

asi1024 and others added 3 commits April 5, 2019 18:37
@asi1024
Copy link
Member

asi1024 commented Apr 8, 2019

@dido1998 Travis test seems to fail for a dtype mismatch. Could you fix it?

@dido1998
Copy link
Contributor Author

I will check it out today.

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

@asi1024 asi1024 left a comment

Choose a reason for hiding this comment

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

TestRelu in test_math.py seem to pass, but test_relu in test_activation.py seems to fails. Could you remove the old test and move the test which you added to test_activation.py?

# Special shapes
chainer.testing.product({
'shape': [(), (0,), (1,), (2, 0, 3), (1, 1, 1), (2, 3)],
'in_dtypes,out_dtype': _in_out_float_dtypes_math_functions,
Copy link
Member

Choose a reason for hiding this comment

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

Why not testing also with integer dtypes?

Suggested change
'in_dtypes,out_dtype': _in_out_float_dtypes_math_functions,
'in_dtypes,out_dtype': _in_out_dtypes_math_functions,

@dido1998
Copy link
Contributor Author

I have made the requested changes.

@asi1024
Copy link
Member

asi1024 commented Apr 11, 2019

Jenkins, test this please.

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 421616a (08e66b8):

@chainer-ci
Copy link
Member

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

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

asi1024 commented Apr 12, 2019

Jenkins, test this please.

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 421616a (bcffb68):

@chainer-ci
Copy link
Member

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

@asi1024
Copy link
Member

asi1024 commented Apr 12, 2019

LGTM. Thank you!

@asi1024 asi1024 merged commit 6015cae into chainer:master Apr 12, 2019
@asi1024 asi1024 added the cat:enhancement Implementation that does not break interfaces. label Apr 12, 2019
@asi1024 asi1024 added this to the v7.0.0a1 milestone Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces. ChainerX Related to ChainerX.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants